-
Notifications
You must be signed in to change notification settings - Fork 7
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
Remove proxy targets #226
Remove proxy targets #226
Conversation
feat: remove native asset interface feat: remove proxy targets from scripts test: remove proxy targets tests build: remove proxy targets from shell scripts build: uninstall permit2 and prb-proxy dep ci: use sepolia for deployment workflows
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.
generally looks good to me, great job @andreivladbrg!
lol how much code was removed.
or should we just updated the changes and keep the 2023-10-19 date?
In the changelog, we will put the date when we ship the npm packages. Speaking of that - when should we do it?
src/libraries/Errors.sol
Outdated
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.
Can you please check that this does not introduce any bytecode changes between this PR and the commit that the auditors have signed off on?
If there are bytecode changes, we will keep the error.
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.
this PR doesn't change the bytecode. however, it will change when we update the git submodules to NPM packages, due to different remappings
also, after the Turing audit, the bytecodes have already changed in both v2-core
and v2-periphery
:
- Use safeTransferFrom in _handleTransfer function #227
- Remove reduntant updateMetadata in withdrawMaxAndTransfer v2-core#735
therefore, this is not a concern.
fyi: we've also had different bytecodes after Spearbit's audit back in the summer
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.
Thanks for checking and for explaining.
this is not a concern.
It is a concern because we need to ask Cantina to update the commits in the audit reports so that we can then link them on Etherscan.
The fact that we introduced some changes because of another auditor is great (it means we have an excuse for changing the bytecode).
yep, the proxy system was quite complex 😅
we first need to publish once we merge sablier-labs/v2-core#734 i can pursue with deployments and publish to answer your question, we will try ASAP |
the PR is ready to be merged now - I will let you do it @andreivladbrg, but please make sure you squash the commits. I suppose that the next step is to install the Node.js version of V2 Core, and subsequently remove the node modules? |
Closes #224
Question, should we update the 2023-10-19 changelog to 2023-11-19? or should we just updated the changes and keep the 2023-10-19 date?