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

Remove proxy targets #226

Merged
merged 4 commits into from
Dec 15, 2023
Merged

Remove proxy targets #226

merged 4 commits into from
Dec 15, 2023

Conversation

andreivladbrg
Copy link
Member

@andreivladbrg andreivladbrg commented Nov 29, 2023

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?

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
Copy link
Member

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

Copy link
Member

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.

Copy link
Member Author

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:

therefore, this is not a concern.

fyi: we've also had different bytecodes after Spearbit's audit back in the summer

Copy link
Member

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

foundry.toml Outdated Show resolved Hide resolved
@andreivladbrg
Copy link
Member Author

lol how much code was removed.

yep, the proxy system was quite complex 😅

when should we do it?

we first need to publish v2-core so that we can install it as NPM package here

once we merge sablier-labs/v2-core#734 i can pursue with deployments and publish

to answer your question, we will try ASAP

@PaulRBerg
Copy link
Member

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?

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.

None yet

2 participants