Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(contracts): deploy MessageProcessor, Tally, and Subsidy contracts after deploy poll #949

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

kittybest
Copy link
Collaborator

@kittybest kittybest commented Jan 4, 2024

Description

This is mainly for deploying Poll with MessageProcessor, Tally, and Subsidy(optional) contracts at the same time.

  • Deploying those contracts by Factory Contracts (PollFactory, MessageProcessorFactory, TallyFactory, SubsidyFactory)
  • To save the size of contract and function call, making Interface Contracts for both Factory Contracts(IMPFactory, IPollFactory, ITallySubsidyFactory) and several smart contracts (IMessageProcessor, IPoll, IVkRegistry)
  • Update Natspec of modified contracts and new contracts

Related issue(s)

close #862.

Confirmation

Copy link

netlify bot commented Jan 4, 2024

Deploy Preview for maci-typedoc ready!

Name Link
🔨 Latest commit 579a53f
🔍 Latest deploy log https://app.netlify.com/sites/maci-typedoc/deploys/65a18b093a8d1d00072a6cde
😎 Deploy Preview https://deploy-preview-949--maci-typedoc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ctrlc03 ctrlc03 force-pushed the refactor/mp-tally-subsidy branch from 1f5d8cb to b9fee47 Compare January 4, 2024 17:42
@kittybest kittybest marked this pull request as ready for review January 10, 2024 11:12
@ctrlc03 ctrlc03 requested a review from crisgarner January 10, 2024 11:18
@kittybest kittybest force-pushed the refactor/mp-tally-subsidy branch from 1db9d5b to 48605c1 Compare January 10, 2024 11:21
Copy link
Collaborator

@ctrlc03 ctrlc03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @kittybest - left few comments here and there

@ctrlc03 ctrlc03 added this to the MACI v1.1.1 refactor milestone Jan 10, 2024
Copy link
Collaborator

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kittybest thanks, just few comments and could you update PR title to match commit conventions?

contracts/contracts/MessageProcessor.sol Show resolved Hide resolved
contracts/contracts/interfaces/IPollFactory.sol Outdated Show resolved Hide resolved
contracts/contracts/interfaces/ITallySubsidyFactory.sol Outdated Show resolved Hide resolved
contracts/contracts/interfaces/IVkRegistry.sol Outdated Show resolved Hide resolved
contracts/tests/constants.ts Show resolved Hide resolved
contracts/ts/deploy.ts Outdated Show resolved Hide resolved
contracts/ts/deploy.ts Outdated Show resolved Hide resolved
@kittybest kittybest force-pushed the refactor/mp-tally-subsidy branch from 48605c1 to 4b903ec Compare January 10, 2024 16:29
@kittybest kittybest changed the title Refactor/mp tally subsidy Refactor/Create Interfaces for contracts, deploy mp, tally, and subsidy right after deployPoll Jan 10, 2024
@kittybest kittybest force-pushed the refactor/mp-tally-subsidy branch 2 times, most recently from c1da034 to fe51956 Compare January 10, 2024 19:57
cli/tests/e2e/e2e.test.ts Outdated Show resolved Hide resolved
cli/tests/e2e/e2e.test.ts Outdated Show resolved Hide resolved
cli/tests/e2e/e2e.test.ts Outdated Show resolved Hide resolved
@kittybest kittybest force-pushed the refactor/mp-tally-subsidy branch 4 times, most recently from c5e2242 to 3d723b1 Compare January 11, 2024 16:43
cli/ts/commands/verify.ts Outdated Show resolved Hide resolved
cli/ts/index.ts Outdated Show resolved Hide resolved
cli/ts/index.ts Outdated Show resolved Hide resolved
@kittybest
Copy link
Collaborator Author

Why are new factory contracts (MessageProcessorFactory, TallyFactory, SubsidyFactory) needed to achieve this?

Tried to deploy contracts inside pollFactory.deploy function but the compiled pollFactory contract size is too large.
image
After discussion with ctrl03, factory contracts might be a neat way to deploy another contracts in a contract, and we don't have to worry about the above error.

But if not using interfaces, simply just pass the factories to maci contract and told the factories to deploy contracts, the following errors were shown:
image

How/why do interface contracts reduce contract size? Or by "size" are you referring to the file size (vs. the contract size)?

Said by ChatGPT ⬇️
Reducing Deployment Gas Costs:
Interfaces help in reducing the deployment gas costs. When interacting with contracts, only the interface needs to be known, not the entire implementation bytecode. This means that developers can deploy smaller contracts that only contain the necessary logic for their specific use case.

But I'm not sure about the deployment costs so far, only know the deployed size and initcode size. We could try to benchmark this.

@kittybest kittybest force-pushed the refactor/mp-tally-subsidy branch from 10905e2 to e5743ba Compare January 11, 2024 20:26
@kittybest kittybest force-pushed the refactor/mp-tally-subsidy branch 2 times, most recently from 22afc9a to 8abb041 Compare January 12, 2024 16:34
@ctrlc03 ctrlc03 mentioned this pull request Jan 12, 2024
3 tasks
@kittybest kittybest force-pushed the refactor/mp-tally-subsidy branch 2 times, most recently from 2a5d653 to dc2822d Compare January 12, 2024 17:36
@ctrlc03 ctrlc03 self-requested a review January 12, 2024 17:56
@kittybest kittybest force-pushed the refactor/mp-tally-subsidy branch 2 times, most recently from 17c0582 to c89fa38 Compare January 12, 2024 18:10
@ctrlc03 ctrlc03 self-requested a review January 12, 2024 18:30
Copy link
Collaborator

@ctrlc03 ctrlc03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes.
Please change the title to match commit conventions, then there's one comment but it's just natspec related

@kittybest kittybest force-pushed the refactor/mp-tally-subsidy branch from c89fa38 to 579a53f Compare January 12, 2024 18:55
@kittybest kittybest changed the title Refactor/Create Interfaces for contracts, deploy mp, tally, and subsidy right after deployPoll refactor(contracts): deploy MessageProcessor, Tally, and Subsidy contracts after deploy poll Jan 12, 2024
@ctrlc03 ctrlc03 requested a review from 0xmad January 12, 2024 19:13
Copy link
Collaborator

@0xmad 0xmad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kittybest thanks, just one minor fix.

@ctrlc03 ctrlc03 enabled auto-merge January 12, 2024 20:39
@ctrlc03 ctrlc03 disabled auto-merge January 12, 2024 20:39
@ctrlc03 ctrlc03 enabled auto-merge January 12, 2024 20:39
@ctrlc03 ctrlc03 merged commit 16925e9 into dev Jan 12, 2024
17 checks passed
@ctrlc03 ctrlc03 deleted the refactor/mp-tally-subsidy branch January 12, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

MessageProcessor and Tally should use a state variable for Poll rather than it being a function parameter
5 participants