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

feat(protocol): allow L1 timelock to be the L2 owner #15358

Closed
wants to merge 10 commits into from

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented Dec 8, 2023

Before this PR, the ownership of the TaikoL2 contract is really not given to the DAO as DAO lives on L1.

This PR introduces a new contract CrossChainOwned.sol that checks the owner address sent signals from L1 to L2 to authorize onlyOwner transactions.

  • @davidtaikocha The L2 genesis script may need some changes as the init() function of TaikoL2 changed.
  • @dantaik will add a test for it.

Copy link

vercel bot commented Dec 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
bridge-ui-v2-a5 ✅ Ready (Inspect) Visit Preview Dec 9, 2023 11:06am
bridge-ui-v2-a6 ✅ Ready (Inspect) Visit Preview Dec 9, 2023 11:06am
1 Ignored Deployment
Name Status Preview Updated (UTC)
bridge-ui-v2-internal ⬜️ Ignored (Inspect) Visit Preview Dec 9, 2023 11:06am

bytes32 approvalHash = _isTransactionApproved(txdata, proof, executor);
if (approvalHash == 0) revert TX_NOT_APPROVED();

(bool success,) = address(this).call(txdata);
Copy link
Member

@davidtaikocha davidtaikocha Dec 9, 2023

Choose a reason for hiding this comment

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

If we let owner() be L1 timelock, shall we modify those onlyOwner methods in TaikoL2 (e.g. function withdraw(address token, address to) external onlyOwner), to make them only executable by the L2 executor here instead of owner()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but we don't override onlyOwner modifier, we overide _checkOwner. It's done in the code.

error TX_REVERTED();
error NOT_CALLABLE();

function executeApprovedTransaction(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this function check its own proof instead of doing things similarly to what checkProcessMessageContext where it's called directly by the bridge contract and just the original sender on the source chain needs to be checked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This new contract doesn't use the Bridge, instead, it interacts with the SignalService directly on the source chain directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for that? Seems like this type of thing will be a common thing that people have to do for these cross layer transactions. It feels a bit strange that even we wouldn't use the standard bridge to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the SignalService is intuitive to me, but we can also use the Bridge. By using the bridge, maybe we can avoid the introduction of the new CrossChainOwned contract. I'll give it a try and then lets compare the two solutions.

@dantaik dantaik marked this pull request as draft December 11, 2023 04:39
@dantaik dantaik closed this Dec 11, 2023
@dantaik dantaik deleted the cross-chain-owner branch January 10, 2024 02:01
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