-
Notifications
You must be signed in to change notification settings - Fork 404
feat(test-execute): Implement pre.deterministic_deploy_contract
#1934
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
base: forks/amsterdam
Are you sure you want to change the base?
feat(test-execute): Implement pre.deterministic_deploy_contract
#1934
Conversation
|
One open question is that, in execute, we will not be able to run two different tests that use the same contract without causing issues. |
|
I've verified this locally using fill and kurtosis, the test can be run over and over and the deployment only happens once. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #1934 +/- ##
================================================
Coverage 86.33% 86.33%
================================================
Files 538 538
Lines 34557 34557
Branches 3222 3222
================================================
Hits 29835 29835
Misses 4148 4148
Partials 574 574
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CPerezz
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.
Important things also:
- Missing max code size validation in execute mode. The filler implementation checks max_code_size (line 176), but execute mode doesn't. This should be patched.
- DETERMINISTIC_DEPLOYMENT_CONTRACT_ADDRESS is defined multiple times.
| DETERMINISTIC_DEPLOYMENT_CONTRACT_ADDRESS = Address( | ||
| 0x4E59B44847B379578588920CA78FBF26C0B4956C | ||
| ) | ||
| DETERMINISTIC_DEPLOYMENT_SIGNER_ADDRESS = Address( | ||
| 0x3FAB184622DC19B6109349B94811493BF2A45362 | ||
| ) | ||
| DETERMINISTIC_DEPLOYMENT_TRANSACTION = Bytes( | ||
| "0xf8a58085174876e800830186a08080b853604580600e600039806000f350fe7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe03601600081602082378035828234f58015156039578182fd5b8082525050506014600cf31ba02222222222222222222222222222222222222222222222222222222222222222a02222222222222222222222222222222222222222222222222222222222222222" | ||
| ) | ||
|
|
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.
What are these hardcoded things? Why do we have a DETERMINISTIC_DEPLOYMENT_CONTRACT_ADDRESS?
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.
Nevermind. I assume we will set Nick's deployer unless we provide another one? If that is the case, we can close this.
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.
IIUC, these info comes from Nick’s deterministic deployer:
DETERMINISTIC_DEPLOYMENT_CONTRACT_ADDRESS: the contract address of Nick’s deployer (the factory address).DETERMINISTIC_DEPLOYMENT_SIGNER_ADDRESS: the address recovered viaecrecoverfrom the randomly generated(v, r, s)values.DETERMINISTIC_DEPLOYMENT_TRANSACTION: the transaction used to deploy Nick’s factory contract.
The information documented here.
| protected=False, | ||
| nonce=0, | ||
| gas_price=0x174876E800, | ||
| gas_limit=0x0186A0, | ||
| to=None, | ||
| value=0, | ||
| data=Bytes( |
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.
Why do we hardcode gas_price here to 0x174876E800 (100 Gwei) ?
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.
The transaction that deploys the proxy must be exactly like this in order to deploy to the expected address: https://github.com/Arachnid/deterministic-deployment-proxy?tab=readme-ov-file#proxy-address. This is just how Nick's method works.
Could you confirm that your miner uses 0x4e59b44847b379578588920ca78fbf26c0b4956c please? I used this as it was the one that was linked in @jochem-brouwer message in the original issue: #1933 (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.
Yes yes. That's Nick's deployer. I was asking about gas price and gas_limit hardcoded here rather than anything else.
The address won';t be affected by that right?
LouisTsai-Csie
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.
Leave some comments! I now have a better understanding of the big picture, will revisit CPerezz' PR to see how to integrate.
| contract_address = compute_deterministic_create2_address( | ||
| salt=salt, initcode=initcode | ||
| ) | ||
| max_code_size = self._fork.max_code_size() | ||
| assert len(deploy_code) <= max_code_size, ( | ||
| f"code too large: {len(deploy_code)} > {max_code_size}" | ||
| ) | ||
|
|
||
| super().__setitem__( | ||
| contract_address, | ||
| Account( | ||
| nonce=1, | ||
| balance=minimum_balance, | ||
| code=deploy_code, | ||
| storage={}, | ||
| ), | ||
| ) |
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.
Some feedback:
In execute remote, we will first check if the contract exists or not, i believe we should do the similar verification here, or we might override the existing contract if we do not have proper salt management. Moreover, in post-state object, it would not raise error when there is duplicate entry.
def test_collision_detection(
blockchain_test: BlockchainTestFiller,
pre: Alloc,
) -> None:
"""
Test collision detection for deterministic deployments.
"""
deploy_code = Op.SSTORE(1, Op.CALLDATALOAD(0))
addr_1 = pre.deterministic_deploy_contract(
deploy_code=deploy_code,
setup_calldata=Hash(0),
label="contract_1",
)
# This would override the addr_1 account
addr_2 = pre.deterministic_deploy_contract(
deploy_code=deploy_code,
setup_calldata=Hash(1),
label="contract_2",
)
# If we are not aware the `addr_1` and `addr_2` are the same, the test might not behave as intended.
assert addr_1 == addr_2, "Contract address is the same"
sender = pre.fund_eoa()
tx_1 = Transaction(
sender=sender,
to=addr_1,
data=Hash(1),
gas_limit=100_000,
)
tx_2 = Transaction(
sender=sender,
to=addr_2,
data=Hash(2),
gas_limit=100_000,
)
post = {
# this is overriden by the `addr_2` account setting, as `addr_1` is the same as `addr_2`.
addr_1: Account(
deploy_code=deploy_code,
storage={
1: 1,
},
),
addr_2: Account(
deploy_code=deploy_code,
storage={
1: 2,
},
),
}
blockchain_test(pre=pre, post=post, blocks=[Block(txs=[tx_1, tx_2])])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.
Moreover, the setup_calldata mechanism is not working for fill but execute remote:
Below is an example:
def test_missing_setup_phase(
blockchain_test: BlockchainTestFiller,
pre: Alloc,
) -> None:
"""
Test collision detection for deterministic deployments.
"""
deploy_code = Op.SSTORE(1, Op.CALLDATALOAD(0))
addr_1 = pre.deterministic_deploy_contract(
deploy_code=deploy_code,
salt=Hash(1),
setup_calldata=Hash(0),
label="contract_1",
)
addr_2 = pre.deterministic_deploy_contract(
deploy_code=deploy_code,
salt=Hash(2),
setup_calldata=Hash(1),
label="contract_2",
)
assert addr_1 != addr_2, "Contract addresses should be different"
sender = pre.fund_eoa()
tx_1 = Transaction(
sender=sender,
to=addr_1,
data=Hash(2),
gas_limit=100_000,
)
post = {
addr_1: Account(
deploy_code=deploy_code,
storage={
1: 2,
},
),
addr_2: Account(
deploy_code=deploy_code,
storage={
1: 1,
},
),
}
blockchain_test(pre=pre, post=post, blocks=[Block(txs=[tx_1])])- expected behavior (Works under execute remote)
- slot 1 in addr_1 increases from 1 to 2
- slot 1 in addr_2 increases stays 1
- Current behavior (In fill mode)
- slot 1 in addr_1 from 0 to 2
- slot 1 in addr_2 stays 0
| new_bytes=len(initcode) | ||
| ) | ||
| deploy_gas_limit += calldata_gas_calculator(data=initcode) | ||
| deploy_gas_limit = min(deploy_gas_limit * 2, 30_000_000) |
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 30M hardcoded limit does not work for Osaka (eip-7825).
| # Limit the gas limit | ||
| deploy_gas_limit = min(deploy_gas_limit * 2, 30_000_000) |
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.
Same argument
f968266 to
7566504
Compare
🗒️ Description
Implements a new function in the
preobject calleddeterministic_deploy_contract, which uses https://github.com/Arachnid/deterministic-deployment-proxy to deploy contracts at deterministic addresses usingCREATE2.The function takes the following parameters:
keccak256( 0xff ++ 0x4E59B44847B379578588920CA78FBF26C0B4956C ++ salt ++ keccak256(init_code))[12:]CREATE2opcode. Defaults to zero.deploy_codeuntil the transactions is sent to the network usingexecute. Can be skipped and the initcode will be auto-generated.🔗 Related Issues or PRs
Closes #1933
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture