-
Notifications
You must be signed in to change notification settings - Fork 355
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
fix: EVM-708 bridge fixes #703
Conversation
Changes to gas cost
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
function setLegacyBaseTokenAssetId(uint256 _chainId) external { | ||
address token = baseToken[_chainId]; | ||
require(token != address(0), "BH: token not set"); | ||
baseTokenAssetId[_chainId] = DataEncoding.encodeNTVAssetId(block.chainid, token); |
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.
should be checked that the chain does not yet have the base token asset id set.
Probably reverting in case the token is set is too much (since the method is public and be frontrun), but at least not overwriting the base token should be done in case some chains have custom asset handler for base token
…a-contracts into kl/l2-legacy-bridge-splicing
@@ -183,6 +183,19 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus | |||
require(token != address(0), "BH: token not set"); | |||
baseTokenAssetId[_chainId] = DataEncoding.encodeNTVAssetId(block.chainid, token); | |||
} | |||
|
|||
/// @notice Used to set the legacy chain address for the upgrade. | |||
/// @notice This has to be used after the BH but before the STM is upgraded. |
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.
I think the comment is not correct, it has to be used after STM is upgraded, since getHyperchainLegacy
is available only on the new one
Coverage after merging kl/l2-legacy-bridge-splicing into kl/sync-layer-reorg will be
Coverage Report |
What ❔
Why ❔
Checklist