Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
draft: ban-deposits-interop first RFC draft #11362
draft: ban-deposits-interop first RFC draft #11362
Changes from 12 commits
3d3e767
0d78e5a
37f17de
4f9621a
65b6804
70b7db0
24deb1c
2e322c9
f463e8a
e90db39
771b319
2838d6e
e40d49f
c89ffa4
ebc225d
198d683
1434e89
8e93fcc
abe646b
13096e8
057fc5b
5e0d64c
2c370f7
da494fd
09c549c
bc562de
a8fd430
0a803e5
78aa2c4
5eaf39d
d5ba5d1
f2d8fb8
ccf505d
a17e357
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit: no newline
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.
@tynes not sure what you mean. do you want a new-line? the other set of 3
unmarshal...
are all clumped togetherThere 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.
The idea of the "sourcehash" is that it helps uniquely identify deposit transactions, because they do not include a nonce.
While technically ok to reuse the same source-hash as the system L1Info deposit (since different tx contents are mixed in), I would prefer us to specify a new type of deposit-source, to really ensure the two deposits get a different tx-hash, and not surprise any users of the source-hash property.
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.
Have we specified/checked how much gas this TX needs conservatively to run?
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.
Changing it to
public
will have effects on prior ecotone usage, since the gas might change (although no explicit arguments to put in memory). I think the ecotone gas limits are conservative and have some buffer for changes like this, and we have tests, but it would still be nice to minimize changes.Maybe instead instead we can change it to
internal
, keep anexternal
function for ecotone that calls theinternal
one, and introduce the new isthmus one asexternal
also? And document the calldata usage between these?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 like having a shared internal while keeping both functions external, what do you think @tynes ?
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.
To address this, we should follow this design doc and introduce a
L1BlockInterop is L1Block
contract