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
EVM support #225
EVM support #225
Changes from 9 commits
7f73d24
1a8a6ec
0c99501
3a3a805
fbb565c
4db769c
b8b0378
d82c91d
c0e3a0f
5dad2cb
8323c2b
5c3004e
f646491
dac4fe4
7cd8dc6
900af71
bcd7a14
27c4675
d2176fc
6dbd64d
eb3543d
aa6ca62
586aa0f
3411fbe
bf0dcf2
5e65fa1
550a270
69f77cd
631055f
cc76ab1
683be9f
318b15b
93931f4
5c78cb6
ca025f7
3dc0125
3e6c3b8
f9ae903
2023b5a
77068ea
d010243
e0122e0
3f8c071
eb6f104
45ae318
def9a09
21c817e
13cc05f
a0e9aac
eff8cb2
29c4b4a
6c185ab
d717807
bacc4cc
380fb8f
39a4bb9
a1793b2
70e1ae2
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.
It would be helpful to have an easy comparator like a
toString()
method. Given the common pattern mapping addresses to values in EVM, I think it would be surprising to find thatEVMAddress
can't be used as a key in a mapping in Cadence.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.
Good idea to add a serialization!
What format would the function return? Hex-encoded? With
0x
prefix or without?IMHO the function should make that clear in the name, so there is no confusion.
toString
is too generalThere 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.
Whichever format is returned should support deserialization into an
EVMAddress
as well. Since hex decoding doesn't support a prefix, I think we should exclude0x
. Though I wonder if foregoing the prefix will cause problems when passing values into EVM calls 🤔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'm finding it frustrating to convert between
[UInt8]
and[UInt8; 20]
when constructing EVMAddress and I think others will as well. For example, given some hex encoded address, I would want to construct an EVMAddress in a script to call a solidity contract. I would expect I could simply:Instead, I have to do:
I think it would be preferable to abstract away the complexities of dealing with the restricted byte array in the struct's initialization.
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 would be great to keep the type-safety, though I see and agree with the UX problems.
If the input type would be a variable-sized array, what would be the behaviour for input that is too short or too long? Developers might make assumptions, which could potentially be different from actual behaviour, and thus lead to costly mistakes.
For converting between constant-size and variable-size arrays, there is actually a Cadence feature request issue open: onflow/cadence#2530
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 think a pre-condition like
bytes.length == 20
inEVMAddress.init
is behaviorally similar to the current implementation. Is that workable?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.
A pre-condition is a dynamic (run-time) check, and not a static one (type-checking time). It would be nice not to have to express the requirement of the length statically so errors can be prevented as early as possible
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.
not familiar with evm world, but I feel like
EVM.run
should return something to Cadence land to be useful somehow.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.
Good point!
I noticed that the FLIP was actually lacking the definition of
EVM.run
, so I added it in 6dbd64d.@ramtinms I noticed that the current implementation does not yet return the documented boolean return value. Also, I think at some point there was a discussion about if the function should:
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.
Unfortunatly it doesn't return anything, but the calls on the COA will return a value.
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.
Right, it makes sense that there is no value returned from executing an EVM transaction (just like executing a transaction on Flow does not return a value).
However, it might still be useful to return a value from the function that runs the transaction which indicates the "result" of the transaction, e.g. if it succeeded or failed, e.g. as a boolean, or a struct which contains more information about the transaction execution.
@ramtinms Could you maybe point me to where in the code (flow-go and geth) where/how an EVM transaction is run and what the result of it is?
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.
The implementation of run currently aborts on any error: https://github.com/onflow/flow-go/blob/645923211a85641ade7d335c773d627390de4901/fvm/evm/handler/handler.go#L77.
Is that the intended behaviour? Then we should update the proposal and remove the current bool return value.
If it isn't: It looks like
BlockView.RunTransaction
returns a transaction result value (https://github.com/onflow/flow-go/blob/645923211a85641ade7d335c773d627390de4901/fvm/evm/emulator/emulator.go#L110-L112),types.Result
, and we could expose (parts of) it to Cadence?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.
Apologies if this is out of scope for this FLIP - given that a BridgedAccount is a resource, what happens in the EVM environment if the resource is destroyed in the Flow environment? Can it be reconstructed? Is there currently a similar case where an address exists and suddenly no longer does in EVM and would this present any unstated consequences?
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.
Destroying the
BridgedAccount
resource is basically equivalent to "throwing away the private key" for an account. Thought technically it could be reconstructed, how should that work – who/what proves ownership of the account?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.
While I take it as implied, I don't believe there are any stated guarantees that a
BridgedAccount
address is unique. From the creation method below, it looks address uniqueness would be handled byInternalEVM
, is that right?As far as reconstruction, I think the resource itself is sufficient to prove ownership and reconstructions isn't a desirable feature. It make sense to me that destruction would be akin to losing the keys.
With the
BridgedAccount
destroyed, wouldn't the address no longer exist on either side? I think I overlayed an interface I saw earlier (not listed in this FLIP) which included a methodEVMAddress.exists(): Bool
which I had assume to returnfalse
once aBridgedAccount
is destroyed.Intuitively, I'd expect the address to persist in the EVM environment post-destruction and some artifact of the address in the Cadence environment - maybe in
InternalEVM
in whatever mechanism prevents duplication?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.
Good point, that should be stated explicitly.
"
InternalEVM
" is an implementation detail and shouldn't be part of the proposal.I now think that I might have misread your question – Where you asking if there is a way to regain access to an EVM account for which the bridged account was destroyed in terms of suggesting that should be possible (how I read the question at first), or are you worried and are implying that shouldn't be possible?
Great question! @ramtinms how do you think this will/should work? This should be documented in the proposal.
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 was asking out of concern, thinking it shouldn't be possible out of concern for implications in the EVM environment.