-
Notifications
You must be signed in to change notification settings - Fork 74
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
Using safe-singleton-factory
for module deployment
#477
Conversation
Pull Request Test Coverage Report for Build 10353559880Details
💛 - Coveralls |
Nope. I think keeping this package as root dependency is the right way we also expect other future modules to get deployed using this factory.
Agreed
As there are no other changes, I think we can do version upgrade and release with other changes in future for each npm package. |
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.
LGTM
|
||
### Official Deployment Address from 3rd party | ||
|
||
- `DaimoP256Verifier` at `0xc2b78104907F722DABAc4C69f826a522B2754De4` ([Source](https://p256.eth.limo/)) |
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.
Not for this PR, but can we create an issue to deploy the DaimoP256Verifier
correctly on chains where it isn't supported?
I don't know how easy it is to make deployment scripts that work with two different CREATE2 deployers, but it should be possible.
Also, we are unnecessarily deploying the DaimoP256Verifier
contract with the Safe Singleton proxy with the current deployment scripts, so those need to be adjusted to not deploy this contract (when not on a test network). This should also be reflected in the issue (I don't want to pull in additional changes to this PR to not slow down the review process even further).
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.
LGTM - can we just create a GH issue to track #477 (comment)?
|
This PR adds the use of
safe-singleton-factory
for deploying themodules
.Note:
allowances
already used thesafe-singleton-factory
for deployment.passkey
version was not updated, as it was not deployed to any network AFAIK.safe-singleton-factory
in the root level. (Should we just install it individually for all 4 modules?)Open Question: Should we update the version of the earlier deployed contracts or just overwrite it in CHANGELOG? Personally think that CHANGELOG should contain the change.
Follow-Up Question: If we are updating the version, then should we make a new release of the npm package?
Fixes #473