-
Notifications
You must be signed in to change notification settings - Fork 75
feat: Deploy PermissionSplitter via forge #412
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
Conversation
Signed-off-by: nicholaspai <npai.nyc@gmail.com>
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
Signed-off-by: nicholaspai <npai.nyc@gmail.com>
This reverts commit af8716b.
…re robust fuzz testing of fallback()/receive() Signed-off-by: nicholaspai <npai.nyc@gmail.com>
adds forge deployment script and other setup along with newly deployed contract at [0x0Bf07B2e415F02711fFBB32491f8ec9e5489B2e7](https://etherscan.io/address/0x0Bf07B2e415F02711fFBB32491f8ec9e5489B2e7#code)
james-a-morris
left a comment
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.
Comment about consolidating some of the hardcoded addresses into a shared solidity file. This will be helpful if we want to deploy this to testnets in the future
| address constant defaultAdmin = 0xB524735356985D2f267FA010D681f061DfF03715; | ||
| address constant hubPool = 0xc186fA914353c44b2E33eBE05f21846F1048bEda; |
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.
Does foundry support shared constants in a similar way to how we handle the hardhat deployments now? I assume the constants would be a solidity file
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.
are you proposing moving these to a separate file? I kind of think its overkill
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.
Are we planning on migrating over to foundry? It may be a good idea to look at this in the future
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.
we will use foundry in all new repos but changing this one over would be pretty difficult because of all the unit tests.
At this point, foundry support is available within the repo so we can use it for new scripts/tests, etc.
mrice32
left a comment
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!
| @@ -0,0 +1,97 @@ | |||
| { | |||
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.
Is this the real deployment? If so, why is it labeled "dry-run"?
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 believe this is the simulation step that happens for forge scripts
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 see. Makes sense.
Co-authored-by: Matt Rice <matthewcrice32@gmail.com>
james-a-morris
left a comment
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
mrice32
left a comment
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!
| @@ -0,0 +1,97 @@ | |||
| { | |||
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 see. Makes sense.
adds forge deployment script and other setup along with newly deployed contract at 0x0Bf07B2e415F02711fFBB32491f8ec9e5489B2e7