Feat deployment alt gas token #20
Conversation
…roll-tech/scroll-contracts into feat-deployment-alt-gas-token
yiweichi
force-pushed
the
feat-deployment-alt-gas-token
branch
from
3b4e1fd to
61ebdd1
Compare
| "start_messenger_balance": null | ||
| "start_messenger_balance": 0 |
There was a problem hiding this comment.
Why 0? L1 messenger balance will not be 0 in a real deployment.
There was a problem hiding this comment.
I just realized it's missing. Added a draft here: #24 probably needs some adjustments.
There was a problem hiding this comment.
I think contracts don't need chain_monitor config, also the bridge-history-config.json
There was a problem hiding this comment.
I think contracts don't need chain_monitor config, also the bridge-history-config.json
Yes, this is mainly because we want to have a single command to generate all config files for scroll-sdk, so here the file https://github.com/scroll-tech/scroll-contracts/blob/feat-deployment-alt-gas-token/docker/scripts/gen-configs.sh will help to do so.
There was a problem hiding this comment.
i see. but i don't like this way. why not generate the configs in scroll-sdk repo?
There was a problem hiding this comment.
Not sure, I guess that because lots of config files need contracts addresses. generate the config files here would be convenient to get/predict those addresses?
Need @Thegaram to confirm.
There was a problem hiding this comment.
Yes, this is a convenient way to generate the configs before actually deploying the contracts. But I'm fine with moving the GenerateConfigs logic into scroll-sdk, or maybe add it to Daniel's cli.
There was a problem hiding this comment.
Why 0? L1 messenger balance will not be 0 in a real deployment.
For this, my original thinking is to burn rest of the deployer balance to all zero address, so we can set chain monitor start_messenger_balance to 0. but in this way L2 contracts deployment gas fee may not be burned.
but with #24 , would be better.
| "chain_monitor": { | ||
| "enabled": false, | ||
| "enabled": true, | ||
| "timeout": 3, |
There was a problem hiding this comment.
Why contract need bridge-history-api, rollup, chain-monitor config @zimpha
There was a problem hiding this comment.
This is mainly because we want to have a single command to generate all config files for scroll-sdk, so here the file https://github.com/scroll-tech/scroll-contracts/blob/feat-deployment-alt-gas-token/docker/scripts/gen-configs.sh will help to do so.
|
|
||
| /// @dev Only execute block if it's requied by alternative gas token mode. | ||
| modifier gasToken(bool gasTokenRequire) { | ||
| if (ALTERNATIVE_GAS_TOKEN_ENABLED != gasTokenRequire) { |
There was a problem hiding this comment.
weird, IMO, if set ALTERNATIVE_GAS_TOKEN_ENABLED=true represents alternative gas token enabled, why need pass gasTokenRequire to check the consistency. if ALTERNATIVE_GAS_TOKEN_ENABLED=fase && gasTokenRequire == false, this check (ALTERNATIVE_GAS_TOKEN_ENABLED != gasTokenRequire) will also verify pass.
There was a problem hiding this comment.
Some functions are only run when alternative gas token is enabled: gasToken(true). Others are run when alternative gas token is disabled: gasToken(false).
You need both, ALTERNATIVE_GAS_TOKEN_ENABLED is the user config for the rollup, while gasTokenRequire is for the script to decide to enable/disable certain functions.
If both are false then the check will evaluate to false (false != false is false), i.e. we will run the function instead of returning early.
Co-authored-by: Morty <[email protected]>
…roll-tech/scroll-contracts into feat-deployment-alt-gas-token
Co-authored-by: zimpha <[email protected]> Co-authored-by: Péter Garamvölgyi <[email protected]>
5 participants
Add alternative gas token contracts and deployment scripts.