-
Notifications
You must be signed in to change notification settings - Fork 264
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
Migration Contracts Changes #777
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more changes, and CI looks to be failing.
CI will fail because we are changing the asset but not adding a new chain ID or deployments. |
@@ -115,6 +118,41 @@ describe('assets/', () => { | |||
} | |||
}); | |||
|
|||
it('should contain the migration contracts networks in all other files', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this can be part of the above test, most of the counting is duplicated anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially thought so, but as the intention of the tests were different, thought to have it separated (one to make sure all the deployments are present in all contracts except the migration one, while the other test makes sure the migration test deployments are also present in other contracts.)
Safe deployments for the migration contracts were added in #777 but retrieving those deployments was not added. This PR adds the same.
This PR adds the new Safe Migration Contracts to the deployments of
v1.4.1
. Corresponding changes have been made to the automated tests and reviews as well.