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

YAS 227 L2-to-L1 Message Receiver #44

Merged
merged 4 commits into from
Mar 19, 2020

Conversation

ben-chain
Copy link
Collaborator

@ben-chain ben-chain commented Mar 18, 2020

Description

Implements YAS 227, L2-to-L1 message receiver in a trusted context to build out dev ex.

Questions

  • I noodled around for a while with data structures. I think it could've been done without a nonce, but I decided having a nonce makes it more similar to the final version where we will need an inclusion proof. Look fine?

Metadata

Fixes

  • Fixes YAS 227

Contributing Agreement

@ben-chain ben-chain changed the title tests passing YAS 227 L2-to-L1 Message Receiver Mar 18, 2020
Copy link

@willmeister willmeister left a comment

Choose a reason for hiding this comment

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

Left a few comments, but looks good!

Question:
Is there ever a time when the sequencer (or some other authorized party) would be able to remove messages from the queue? May be nice to reduce state and get a gas rebate.

uint nonce
);

struct EnqueuedL2ToL1Message {

Choose a reason for hiding this comment

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

If this will be passed between multiple contracts, should we put it in DataTypes.sol?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I can tell, it is only gonna show up here, so that's where I put it.


struct EnqueuedL2ToL1Message {
dt.L2ToL1Message message;
uint blockEnqueued;

Choose a reason for hiding this comment

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

This is L1 block, right?

}

address public trustedSequencer;
uint public messageDelay;

Choose a reason for hiding this comment

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

What unit is this in? Seconds?

blockEnqueued: blockNum
});
messageNonce += 1;
emit L2ToL1MessageEnqueued(

Choose a reason for hiding this comment

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

Will the event have a blocknum accessible by default? Also, will we want the nonce in this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, that was supposed to be the nonce!

}

function verifyL2ToL1Message(dt.L2ToL1Message memory _message, uint _nonce) public view returns (bool) {
// The enqueued message at the given nonce mudt match the _message being verified

Choose a reason for hiding this comment

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

Suggested change
// The enqueued message at the given nonce mudt match the _message being verified
// The enqueued message at the given nonce must match the _message being verified

uint blockEnqueued;
}

address public trustedSequencer;

Choose a reason for hiding this comment

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

How do you feel about removing the word trusted? My rationale is that it's implied and may draw unnecessary negative attention.

@ben-chain
Copy link
Collaborator Author

Is there ever a time when the sequencer (or some other authorized party) would be able to remove messages from the queue? May be nice to reduce state and get a gas rebate.

This is a little tricky, since the context for this contract is that it is really there to " simulate" what an actual rollup contract would do. And in that context, the messages can't be removed--but they also don't appear on the main chain other than implicitly via a state root. So for now, I think I'm not gonna add that ability as it would be deprecated quickly anyway.

@ben-chain ben-chain merged commit fcd9919 into master Mar 19, 2020
@gakonst gakonst deleted the feat/YAS227/mocked-l2-to-l1-message-reciever branch March 18, 2021 15:01
protolambda added a commit to protolambda/optimism that referenced this pull request May 1, 2022
…down-lint

Add markdown linting and formatting with markdownlint
@github-actions github-actions bot mentioned this pull request Sep 20, 2022
bap2pecs pushed a commit to babylonlabs-io/optimism that referenced this pull request Jul 31, 2024
blockchaindevsh added a commit to blockchaindevsh/optimism that referenced this pull request Sep 25, 2024
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.

2 participants