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

Test refactoring for new-arc #29

Merged
merged 25 commits into from
Mar 25, 2022
Merged

Test refactoring for new-arc #29

merged 25 commits into from
Mar 25, 2022

Conversation

drahrealm
Copy link
Collaborator

@drahrealm drahrealm commented Mar 22, 2022

Related Issues

Changes

  • Based off the new-arc branch:
    • Added additional test scenarios
    • Added solidity-coverage package to help identifying possible missing (and important) test scenarios
    • Added helper method to streamline event args and signature assertions (via validateEvent) and apply it to all existing event tests
    • Change majority of immutable state variables to constant instead (ie. they already are assigned on declaration)
    • Remove redundant/duplicate calls to all constant state variables in tests
    • Split PirexCvx unit tests into 3 smaller groups based on action flow (base, main, reward)
    • Use folder-wide before hook to setup required variables commonly used for all tests

Notes

  • Run npm i for the first time testing the PR due to new packages

@drahrealm drahrealm changed the base branch from main to new-arc March 22, 2022 17:31
uint8 public immutable PERCENT_DENOMINATOR = 100;
bytes32 public immutable FEE_DISTRIBUTOR_ROLE =
uint8 public constant PERCENT_DENOMINATOR = 100;
bytes32 public constant FEE_DISTRIBUTOR_ROLE =
Copy link
Contributor

@kphed kphed Mar 23, 2022

Choose a reason for hiding this comment

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

I changed the other non-32 byte variables to constants, but for FEE_DISTRIBUTOR_ROLE, we should keep it as an immutable variable even if we don't set it in the constructor:

For a constant variable, the expression assigned to it is copied to all the places where it is accessed and also re-evaluated each time. This allows for local optimizations. Immutable variables are evaluated once at construction time and their value is copied to all the places in the code where they are accessed. For these values, 32 bytes are reserved, even if they would fit in fewer bytes. Due to this, constant values can sometimes be cheaper than immutable values.

ethereum/solidity#4024

It seems that constants may be more efficient in cases where the variable does not need 32 bytes allocated (would not be the case here).

Also, since constants are re-evaluated every time, wouldn't that increase runtime costs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both constant and immutable vars are replaced by their values at execution, so usual expensive gas cost from reading state vars won't apply here. Did some test run with gas reporter and I can confirm that immutable does cost a bit less gas than constant for bytes32, but more a bit more expensive for those smaller than bytes32 size. Good find 👍

@kphed kphed force-pushed the new-arc branch 2 times, most recently from 6062b19 to 0b12116 Compare March 24, 2022 22:38
test/FeePool.ts Show resolved Hide resolved
@kphed kphed merged commit 0030d13 into new-arc Mar 25, 2022
@kphed kphed mentioned this pull request Mar 25, 2022
1 task
kphed added a commit that referenced this pull request Mar 31, 2022
kphed added a commit that referenced this pull request Mar 31, 2022
kphed added a commit that referenced this pull request Mar 31, 2022
kphed added a commit that referenced this pull request Mar 31, 2022
kphed added a commit that referenced this pull request Mar 31, 2022
kphed added a commit that referenced this pull request Mar 31, 2022
kphed added a commit that referenced this pull request Mar 31, 2022
@kphed kphed deleted the new-arc-test-refactor branch March 31, 2022 17:22
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.

2 participants