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

feat(contracts) - add custom errors #764

Merged
merged 7 commits into from
Nov 2, 2023
Merged

feat(contracts) - add custom errors #764

merged 7 commits into from
Nov 2, 2023

Conversation

ctrlc03
Copy link
Collaborator

@ctrlc03 ctrlc03 commented Oct 22, 2023

  • change in contracts
  • fix tests
  • upgrade hardhat/etheres libraries
  • remove jest and use mocha/chai for catching custom errors

fix #761
fix #774

@ctrlc03 ctrlc03 linked an issue Oct 22, 2023 that may be closed by this pull request
@ctrlc03 ctrlc03 self-assigned this Oct 22, 2023
@ctrlc03 ctrlc03 linked an issue Oct 23, 2023 that may be closed by this pull request
@ctrlc03 ctrlc03 marked this pull request as ready for review October 23, 2023 18:17
@baumstern
Copy link
Member

Just want to get a better grip on the benefits. I'm asking purely to learn:

  • Could you shed some light on the gas fee savings with these custom errors (Is it less than $1, between $1-$10, or more than $10?)?
  • I saw custom errors like error NotInit();—are these accompanied by custom messages too? I didn't spot any.
  • What are the advantages of using mocha/chai over jest for this particular case?

@ctrlc03
Copy link
Collaborator Author

ctrlc03 commented Oct 30, 2023

  • shed some light on the gas fee savings with these custom errors (Is it less than $1, between $1-$10, or more than $10?)?

Just want to get a better grip on the benefits. I'm asking purely to learn:

  • Could you shed some light on the gas fee savings with these custom errors (Is it less than $1, between $1-$10, or more than $10?)?
  • I saw custom errors like error NotInit();—are these accompanied by custom messages too? I didn't spot any.
  • What are the advantages of using mocha/chai over jest for this particular case?
  1. this should explain in deeper details how custom errors work: https://soliditylang.org/blog/2021/04/21/custom-errors/. hard to put a price tag on the savings, they however are savings.
  2. That is the error message - NotInit (the contract was not initialized already) -> can change to NotInitialized() if you think it's not clear enough.
    • hardhat tests run using mocha not jest -> I have tried to make some changes to the tests, for instance running using two signers, however I was unsuccessful when using the code as it is now. I believe starting to run the tests directly with hardhat will allow us to do much more thorough tests, as well as simplify the code/scripts down the line -> the next step is to actually improve the tests and all the deploy scripts which look a bit rudimental to me.
    • custom errors need chai matchers

Copy link
Member

@baumstern baumstern left a comment

Choose a reason for hiding this comment

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

It looks like the main problem this PR wants to address is 'Contracts inheriting the MACI contracts could fail to deploy due to hitting contract size limit,' as mentioned in #761. But we shouldn't compromise on the clarity of error messages. One middle-ground solution could be to use concise error codes in the contract and provide inline comments for additional context. Another approach would be to maintain an external error code table for comprehensive explanations, making it easier for debugging without affecting contract size.

contracts/contracts/MACI.sol Outdated Show resolved Hide resolved
contracts/contracts/MACI.sol Outdated Show resolved Hide resolved
contracts/contracts/MACI.sol Outdated Show resolved Hide resolved
contracts/contracts/MACI.sol Show resolved Hide resolved
vkRegistry.owner() == owner(),
"MACI: VkRegistry owner incorrectly set"
);
if (pollFactory.owner() != address(this)) revert WrongPollOwner();
Copy link
Member

Choose a reason for hiding this comment

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

The term 'WrongPollOwner' is ambiguous and could imply various issues—whether the owner is not set, incorrectly set, or unauthorized. This lack of specificity contrasts with the original message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in Solidity if a value is not set it would be set as the default value -> in the address case it would be address(0). Now let's evaluate: address(0) != address(this) -> this would evaluate as false -> address zero (which is not set) is the wrong poll owner.
Given the contracts inherit the Ownable library, once a contract is deployed, the contract owner becomes the address deploying -> it could only be "not set" (which really is set to address(0)) if they were to renounce ownership -> hence stil incorrect owner.

contracts/contracts/trees/AccQueue.sol Outdated Show resolved Hide resolved
contracts/contracts/trees/AccQueue.sol Outdated Show resolved Hide resolved
contracts/contracts/trees/AccQueue.sol Outdated Show resolved Hide resolved
contracts/contracts/trees/AccQueue.sol Show resolved Hide resolved
contracts/contracts/trees/AccQueue.sol Outdated Show resolved Hide resolved
@baumstern
Copy link
Member

This table compares the deployed sizes and shows how much they have been reduced by introducing custom errors:

| Contract Name                       | Original Size (KB) | Improved Size (KB) | Reduction (KB)  |
|-------------------------------------|--------------------|--------------------|-----------------|
| AccQueueBinary0                     | 7.100              | 6.356              | -0.744          |
| AccQueueBinaryMaci                 | 7.100              | 6.356              | -0.744          |
| AccQueueQuinary0                   | 7.551              | 6.797              | -0.754          |
| AccQueueQuinaryBlankSl             | 7.551              | 6.797              | -0.754          |
| AccQueueQuinaryMaci                | 7.551              | 2.061              | -5.490          |
| Address                            | 0.086              | 0.084              | -0.002          |
| CommonUtilities                    | 0.063              | 0.062              | -0.001          |
| ConstantInitialVoiceCreditProxy    | 0.372              | 0.363              | -0.009          |
| DomainObjs                         | 2.110              | 0.061              | -2.049          |
| ERC20                              | 2.141              | 2.091              | -0.050          |
| ERC721                             | 4.575              | 4.468              | -0.107          |
| FreeForAllGatekeeper               | 0.425              | 0.415              | -0.010          |
| Hasher                             | 1.820              | 1.777              | -0.043          |
| HasherBenchmarks                   | 1.926              | 1.881              | -0.045          |
| IMessage                           | 0.063              | 0.062              | -0.001          |
| IPubKey                            | 0.063              | 0.062              | -0.001          |
| MACI                               | 7.870              | 7.103              | -0.767          |
| Math                               | 0.086              | 0.084              | -0.002          |
| MessageProcessor                   | 7.831              | 7.465              | -0.366          |
| MockVerifier                       | 0.966              | 0.938              | -0.028          |
| Pairing                            | 0.086              | 0.084              | -0.002          |
| Params                             | 0.063              | 0.062              | -0.001          |
| PoL1                               | 7.679              | 7.002              | -0.677          |
| PollDeploymentParams               | 0.063              | 0.062             | -0.001         |
| PoLFactory                         | 21.637             | 19.802             | -1.835          |
| PoseidonT3                         | 0.281              | 0.274              | -0.007          |
| PoseidonT4                         | 0.279              | 0.272              | -0.007          |
| PoseidonT5                         | 0.279              | 0.272              | -0.007          |
| PoseidonT6                         | 0.279              | 0.272              | -0.007          |
| SafeERC20                          | 0.086              | 0.084              | -0.002          |
| SignUpToken                        | 5.435              | 5.308              | -0.127          |
| SignUpTokenGatekeeper              | 1.610              | 1.294              | -0.316          |
| SnarkCommon                        | 0.063              | 0.062              | -0.001          |
| SnarkConstants                     | 0.063              | 0.062              | -0.001          |
| Strings                            | 0.086              | 0.084              | -0.002          |
| Subsidy                            | 6.460              | 6.211              | -0.249          |
| Tally                              | 6.286              | 6.001              | -0.285          |
| TopupCredit                        | 3.361              | 3.282              | -0.079          |
| Utilities                          | 2.885              | 2.817              | -0.068          |
| Verifier                           | 3.743              | 3.655              | -0.088          |
| VkRegistry                         | 6.506              | 6.005              | -0.501          |

@ctrlc03 ctrlc03 force-pushed the feat/custom-errors branch from 279cebc to 842ce1a Compare November 1, 2023 09:45
@ctrlc03 ctrlc03 requested a review from baumstern November 1, 2023 18:25
Copy link
Member

@baumstern baumstern left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to go through the comments and making those adjustments. LGTM!

@ctrlc03 ctrlc03 force-pushed the feat/custom-errors branch from 842ce1a to 0f446ee Compare November 2, 2023 09:15
@ctrlc03 ctrlc03 merged commit 14629e3 into dev Nov 2, 2023
8 checks passed
@ctrlc03 ctrlc03 deleted the feat/custom-errors branch November 2, 2023 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

can publish max messages + 1 Reduce contract size by using custom errors instead of error messages
2 participants