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

chore: remove vyper signature from runtime #3471

Merged

Conversation

charles-cooper
Copy link
Member

it's going at the end of initcode instead, which is cheaper but still possible to pick up the vyper version by looking at the create tx.

What I did

How I did it

How to verify it

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

it's going at the end of initcode instead, which is cheaper but still
possible to pick up the vyper version by looking at the create tx.
@charles-cooper charles-cooper requested a review from fubuloubu May 30, 2023 20:04
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Needs some QA by deploying on a public testnet with brownie or ape (preferrably both)

@codecov-commenter
Copy link

codecov-commenter commented May 30, 2023

Codecov Report

Merging #3471 (4394ba4) into master (66b9670) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master    #3471   +/-   ##
=======================================
  Coverage   89.30%   89.30%           
=======================================
  Files          84       84           
  Lines       10792    10793    +1     
  Branches     2461     2461           
=======================================
+ Hits         9638     9639    +1     
  Misses        756      756           
  Partials      398      398           
Impacted Files Coverage Δ
vyper/compiler/output.py 91.66% <ø> (ø)
vyper/compiler/phases.py 91.07% <100.00%> (+0.08%) ⬆️
vyper/ir/compile_ir.py 93.76% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@charles-cooper
Copy link
Member Author

Needs some QA by deploying on a public testnet with brownie or ape (preferrably both)

@fubuloubu what exactly do you want to see here? the issue i am anticipating is that the contract will not verify anyway because the bytecode will not match 0.3.9 (or any other released version of vyper)

@fubuloubu
Copy link
Member

Needs some QA by deploying on a public testnet with brownie or ape (preferrably both)

@fubuloubu what exactly do you want to see here? the issue i am anticipating is that the contract will not verify anyway because the bytecode will not match 0.3.9 (or any other released version of vyper)

Doesn't need to be verified, just make sure that regular and blueprint deployments work on Sepolia or something

@charles-cooper
Copy link
Member Author

Doesn't need to be verified, just make sure that regular and blueprint deployments work on Sepolia or something

using titanoboa, i was able to deploy test contracts with the latest commit on this PR (4394ba4) to sepolia:
carbon - 2023-06-03T001049 746

you can check that the runtime contracts don't contain the vyper signature:
https://sepolia.etherscan.io/address/0x5941ac081899ff0e9941ea4c91eb3cf6a57e2d04
https://sepolia.etherscan.io/address/0x98b2beaf877ad73e010a6ad264c88432411b1b0a

and the blueprint contract does:
https://sepolia.etherscan.io/address/0x2dbf66af9b979245710d13ac5ea79778759d1ca2

all three contain the signature in the creation traces:
https://sepolia.etherscan.io/tx/0x8ca2c12ab395df5cc1f2583fa60350078894940685b51a726d4d0f40d4a45c94
https://sepolia.etherscan.io/tx/0x12c78aefc0e4f2552339d22407c9683c58cab7d3824c07dc98b750a297531cb9
https://sepolia.etherscan.io/tx/0xc55d2dde74b36f274f811dc6ce42044d5a749b6cf871e984b31dc41fa03240e8

also note that a recent dashboard created to check vyper deployments in fact already uses the creation trace as opposed to the runtime signature (presumably because immutables are appended after signature in the bytecode): https://dune.com/queries/2555852/4221288

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.

3 participants