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
Transact from sub to eth #1145
base: bridge-next-gen
Are you sure you want to change the base?
Transact from sub to eth #1145
Changes from 4 commits
fe7a814
89c9e3f
5823492
00c5eed
ebc8986
3d18d1a
8ceb5ee
42f9e07
0a8dc1e
69305f8
bd88ed8
f6beeb1
add22c2
9019ab4
1fb852d
b89cc6c
606e867
539c180
fe513c7
7880d46
27c14e9
14a40cf
0510064
c3b7817
b7e85ca
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.
We should understand how using zexcessivelySafeCall` can still fail? And potentially have tests for at least one case to make sure we don't poisen pill the inbound queue.
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.
Yeah, an unsafe call could be dangerous so we need to be very careful here. Except that I would also like to introduce white-list access control, something like the SafeCallFilter but on Ethereum side.
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.
ebc8986
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.
My thoughts about this:
I think
excessivelySafeCall
(used by Nomad) is too complicated for our needs. When issuing a call, we are only interested in whether the call succeeded or not. Thereturndata
we can ignore. For this reason, I prefer that we use this safe call solution: https://github.com/ethereum-optimism/optimism/blob/b36eb5515cc2a34a15383b2eee488dbac83d6caf/packages/contracts-bedrock/src/libraries/SafeCall.sol#L12I am optimistic we've accounted for all the ways a contract call can be unsafe. This includes guards against re-entrancy (nonce check), gas limits, and returnbomb attacks. Therefore I don't see a reason to include a safe call filter. If we think a call can still be unsafe, we need to understand why. For now I would just remove the safe call filter.
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.
@yrong How does SafeCallFilter work? Will we have to restrict allowing transact to only explicitly specified contracts and/or functions? This won't work - we 100% need permissionless support for anyone to integrate transacting with any contract and any function without permission.
And yeah agree with @vgeddes on just ignoring all returndata
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.
It's not by us or requires governance from the relaychain, but instead from the Parachain sovereign through xcm(i.e. require another call added in our system pallet), they can increase the whitelist step by step entirely under their control.
In this way, we can release the transact support without introducing potential risk.
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.
Ignore returndata 0a8dc1e
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.
Allow arbitray transact without any black(white)-list 606e867
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.
We're no longer using this, can we remove it?
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.
0510064