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

Unit Test Multisig Task Contract #497

Merged

Conversation

ElliotFriedman
Copy link
Contributor

Unit Test Multisig Contract

Add the most unit test like contracts we can for the MultisigTask file

Tests

These tests look for calldata length, targets and values to make sure they all line up.

Metadata

#480

test/MultisigTask.t.sol Outdated Show resolved Hide resolved
@blmalone
Copy link
Contributor

Same question as I asked in discord, is this test adding anymore value that #485 isn't already adding?

@ElliotFriedman
Copy link
Contributor Author

Same question as I asked in discord, is this test adding anymore value that #485 isn't already adding?

I wanted to make this a unit test version of #485 but it became clear that the following things would need to be true for the unit test to work without being on mainnet:

  1. we have an actual deployed safe we are talking to
  2. we set the ChainId to either mainnet or sepolia so that the AddressRegistry contract will work. if we have a local chainid 31337, then it does not work as the constructor of AddressRegistry will revert
  3. then we'd need a mock contract to call and do some action

@prateek105
Copy link
Contributor

Same question as I asked in discord, is this test adding anymore value that #485 isn't already adding?

I think the value that these tests are adding is that these are covering more untested branches in the MultisigTask contract. @ElliotFriedman can confirm. @blmalone

Copy link
Contributor

@prateek105 prateek105 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@blmalone blmalone left a comment

Choose a reason for hiding this comment

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

This is my first pass. I still have to finish going through all the files.

test/mock/MockMultisigTask.sol Outdated Show resolved Hide resolved
test/mock/MockMultisigTask.sol Outdated Show resolved Hide resolved
test/mock/MockMultisigTask.sol Outdated Show resolved Hide resolved
test/mock/MockMultisigTask.sol Outdated Show resolved Hide resolved
test/mock/MockMultisigTask.sol Outdated Show resolved Hide resolved
src/fps/task/MultisigTask.sol Show resolved Hide resolved
test/MultisigTask.t.sol Outdated Show resolved Hide resolved
test/MultisigTask.t.sol Outdated Show resolved Hide resolved
test/MultisigTask.t.sol Outdated Show resolved Hide resolved
blmalone
blmalone previously approved these changes Jan 31, 2025
Copy link
Contributor

@blmalone blmalone left a comment

Choose a reason for hiding this comment

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

It's looking great. I've completed my final pass. I approved on the basis the comments are addressed here.

test/MultisigTask.t.sol Outdated Show resolved Hide resolved
test/task/mock/MockTarget.sol Outdated Show resolved Hide resolved
test/MultisigTask.t.sol Outdated Show resolved Hide resolved
@blmalone blmalone enabled auto-merge January 31, 2025 20:09
@blmalone blmalone added this pull request to the merge queue Jan 31, 2025
Merged via the queue into ethereum-optimism:main with commit fccfe76 Jan 31, 2025
16 checks passed
@blmalone blmalone deleted the feat/unit-test-multisig-task branch January 31, 2025 21:14
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.

3 participants