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

Updated to safe 1.4.1 #7

Merged
merged 6 commits into from
Nov 7, 2023
Merged

Conversation

0xKitsune
Copy link
Contributor

@0xKitsune 0xKitsune commented Jul 1, 2023

I updated the submodule for the safe-contracts submodule from v1.3.0 to v1.4.1.

The createProxy() function seems to have been removed so I replaced it with the createProxyWithNonce() function.

This requires that a saltNonce is passed in and currently in the PR, if advanced parameters are not specified, the saltNonce is set to 0. I think that this should be changed and can be a bit of a footgun if a user calls _setupSafe() multiple times without specifying the salt, an outcome other than what the user probably expected would occur.

I figured I would open this PR and have a conversation here as to what might be the best change to make (whether requiring the user to pass in the saltNonce, dynamically generating the saltNonce without collision or something else).

@0xKitsune 0xKitsune changed the title Chore: updated to safe 1.4.1 Updated to safe 1.4.1 Jul 1, 2023
@colinnielsen
Copy link
Owner

Thanks for opening this @0xKitsune, I will review and merge once we solve this other PR.

RE: your concern, I think it's reasonable to have the SafeTestTools contract store it's own counter (safeInstanceCount) which increments every safe instance created.
The salt nonce could just be:

keccak256(abi.encodePacked("SAFE_TEST_TOOLS", safeInstanceCount))

Thoughts?

@0xKitsune
Copy link
Contributor Author

This sounds like a good approach. While an edge case, there is a possibility for a user to initialize a new safe with _setupSafe() and then initialize another safe with advanced parameters, not realizing that they used the same salt. I would imagine this is unlikely to happen and good documentation will probably suffice. Let me know what you think.

@colinnielsen
Copy link
Owner

This sounds like a good approach. While an edge case, there is a possibility for a user to initialize a new safe with _setupSafe() and then initialize another safe with advanced parameters, not realizing that they used the same salt. I would imagine this is unlikely to happen and good documentation will probably suffice. Let me know what you think.

Extremely unlikely, but I like your adversarial mindset :)

@colinnielsen
Copy link
Owner

@0xKitsune merging this in - thanks again.
Next time, make sure to not commit any code formatted with your config of forge fmt. It was a pain to fix some stylistic merge conflicts

@colinnielsen colinnielsen merged commit daae3df into colinnielsen:main Nov 7, 2023
1 check passed
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