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

Upgradable L1TokenBridge and L2TokenBridge #7

Merged
merged 7 commits into from
Oct 8, 2024
Merged

Upgradable L1TokenBridge and L2TokenBridge #7

merged 7 commits into from
Oct 8, 2024

Conversation

telome
Copy link
Contributor

@telome telome commented Oct 1, 2024

No description provided.

@DaiFoundation-DevOps
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


telome seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@telome telome marked this pull request as ready for review October 1, 2024 16:31
@@ -44,6 +47,7 @@ contract L2TokenBridgeSpell {
l2Bridge = L2TokenBridgeLike(l2Bridge_);
}

function upgradeToAndCall(address newImp, bytes memory data) external { l2Bridge.upgradeToAndCall(newImp, data); }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I chose to ignore that this function is normally payable to keep things simpler, as it doesn't seem useful to be able to pass value to the initialization function (and the L2GovernanceRelay doesn't explicitly supports receiving ether, though there is still the selfdestruct workaround).

README.md Show resolved Hide resolved
deploy/L2TokenBridgeSpell.sol Show resolved Hide resolved
deploy/TokenBridgeInit.sol Show resolved Hide resolved
src/L1TokenBridge.sol Outdated Show resolved Hide resolved
src/L2TokenBridge.sol Show resolved Hide resolved
@@ -108,5 +114,6 @@ library TokenBridgeInit {
dss.chainlog.setAddress(cfg.govRelayCLKey, address(l1GovRelay));
dss.chainlog.setAddress(cfg.escrowCLKey, address(escrow));
dss.chainlog.setAddress(cfg.l1BridgeCLKey, address(l1Bridge));
dss.chainlog.setAddress(cfg.l1BridgeImpCLKey, l1BridgeInstance.bridgeImp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dss.chainlog.setAddress(cfg.l1BridgeImpCLKey, l1BridgeInstance.bridgeImp);
dss.chainlog.setAddress(cfg.l1BridgeImpCLKey, l1BridgeInstance.bridgeImp);

Or otherwise align the entire column.

Copy link
Contributor

@oldchili oldchili left a comment

Choose a reason for hiding this comment

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

minor styling change suggested, but otherwise LGTM

Copy link
Contributor

@sunbreak1211 sunbreak1211 left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Contributor

@sunbreak1211 sunbreak1211 left a comment

Choose a reason for hiding this comment

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

Looks good

@telome telome merged commit 0f93550 into dev Oct 8, 2024
2 of 3 checks passed
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.

4 participants