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

Using safe-singleton-factory for module deployment #477

Merged
merged 11 commits into from
Aug 13, 2024
Merged

Conversation

remedcu
Copy link
Member

@remedcu remedcu commented Aug 5, 2024

This PR adds the use of safe-singleton-factory for deploying the modules.

Note:

  • allowances already used the safe-singleton-factory for deployment.
  • passkey version was not updated, as it was not deployed to any network AFAIK.
  • Lock file was updated due to the addition of 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

@remedcu remedcu self-assigned this Aug 5, 2024
@coveralls
Copy link

coveralls commented Aug 5, 2024

Pull Request Test Coverage Report for Build 10353559880

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 10214202563: 0.0%
Covered Lines: 51
Relevant Lines: 51

💛 - Coveralls

@remedcu remedcu marked this pull request as ready for review August 5, 2024 09:32
@remedcu remedcu requested a review from a team as a code owner August 5, 2024 09:32
@remedcu remedcu requested review from nlordell, akshay-ap and mmv08 and removed request for a team August 5, 2024 09:32
@akshay-ap
Copy link
Member

Should we just install it individually for all 4 modules?

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.

Personally think that CHANGELOG should contain the change.

Agreed

If we are updating the version, then should we make a new release of the npm package?

As there are no other changes, I think we can do version upgrade and release with other changes in future for each npm package.

Copy link
Member

@akshay-ap akshay-ap left a comment

Choose a reason for hiding this comment

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

LGTM

modules/4337/CHANGELOG.md Outdated Show resolved Hide resolved
@remedcu remedcu requested a review from nlordell August 12, 2024 09:14
modules/4337/CHANGELOG.md Outdated Show resolved Hide resolved

### Official Deployment Address from 3rd party

- `DaimoP256Verifier` at `0xc2b78104907F722DABAc4C69f826a522B2754De4` ([Source](https://p256.eth.limo/))
Copy link
Collaborator

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).

@remedcu remedcu requested a review from nlordell August 12, 2024 14:12
Copy link
Collaborator

@nlordell nlordell left a 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)?

@remedcu remedcu mentioned this pull request Aug 13, 2024
@remedcu remedcu merged commit b3c5e66 into main Aug 13, 2024
14 checks passed
@remedcu remedcu deleted the using-factory branch August 13, 2024 15:01
@github-actions github-actions bot locked and limited conversation to collaborators Aug 13, 2024
@remedcu
Copy link
Member Author

remedcu commented Aug 13, 2024

LGTM - can we just create a GH issue to track #477 (comment)?

#481

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Safe Singleton Factory for Deployments
4 participants