-
Notifications
You must be signed in to change notification settings - Fork 23
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
Conversation
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.
Overall I think we'll need to provide more colour and detail, perhaps context, where needed since there's a degree of assumed knowledge that might escape some readers. Otherwise mostly some smaller copy edits and tidy ups
Co-authored-by: j pimmel <[email protected]>
Co-authored-by: j pimmel <[email protected]>
Co-authored-by: j pimmel <[email protected]>
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.
👌
Co-authored-by: Bastian Müller <[email protected]> Co-authored-by: Greg Santos <[email protected]>
protocol/20231116-evm-support.md
Outdated
``` | ||
access(all)contract EVM { | ||
|
||
access(all) resource BridgedAccount { |
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?
where an address exists and suddenly no longer does in EVM
Can you elaborate? Address on which 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.
Thought technically reconstructed, how should that work – who/what proves ownership of the account?
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 by InternalEVM
, 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.
Can you elaborate? Address on which side?
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 method EVMAddress.exists(): Bool
which I had assume to return false
once a BridgedAccount
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.
I don't believe there are any stated guarantees that a
BridgedAccount
address is unique.
Good point, that should be stated explicitly.
it looks address uniqueness would be handled by
InternalEVM
, is that right?
"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?
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.
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.
protocol/20231116-evm-support.md
Outdated
access(all) contract EVM { | ||
|
||
/// EVMAddress is an EVM-compatible address | ||
access(all) struct EVMAddress { |
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 that EVMAddress
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 general
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.
What format would the function return? Hex-encoded? With 0x prefix or without?
Whichever format is returned should support deserialization into an EVMAddress
as well. Since hex decoding doesn't support a prefix, I think we should exclude 0x
. Though I wonder if foregoing the prefix will cause problems when passing values into EVM calls 🤔
protocol/20231116-evm-support.md
Outdated
access(all) | ||
fun main(rlpEncodedTransaction: [UInt8], coinbaseBytes: [UInt8; 20]) { | ||
let coinbase = EVM.EVMAddress(bytes: coinbaseBytes) | ||
EVM.run(tx: rlpEncodedTransaction, coinbase: coinbase) |
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:
- Not return a result value indicating success, but rather cause the outer Flow/Cadence transaction to abort (not proposing it, just documenting this was considered, and noting it should be part of the proposal;)
- Return a result value, but not just a boolean value, but e.g. a struct with more information (reason / error message? what else could be returned?)
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?
Update 20231116-evm-support.md
This commit contains updates that adds `dryRun` function, changes event type ID, changes event field values, adds secure random source Cadence arch method, adds `batchRun` function.
@franklywatson 13cc05f changed the status to implemented, but the FLIP hasn't even been accepted yet? See https://github.com/onflow/flips?tab=readme-ov-file#proposal-states |
@KshitijChaudhary666 what is the process for moving this forward? is there a meeting to present it? |
@ramtinms This proposal probably lies in between the Cadence Language and Execution Working Group and Flow Core-Protocol Working Group. Maybe indicate that this proposal is ready for a final round of feedback, and will get approved unless there are any further concerns brought forward. For example, post in Discord and in the forum. We can maybe schedule a working group meeting soon to have a last discussion, and then get it approved (so far sentiment has been positive). |
@ramtinms Is the FLIP up-to-date with the latest changes made in flow-go? |
Hey - simply talking from a FLIP-lifecycle-perspective - presenting the FLIP in the next WG meeting for a final feedback like @turbolent proposed does make sense to me. Once the author is confident of the community sentiment, the status of the FLIP could be changed to "accepted" and thereafter go to implementation |
@turbolent I believe it has all the recent changes. @sideninja any thing we might be missing? |
Discord conversation on getting this FLIP over the finish line and a Working Group Meeting for a final discussion and getting the FLIP formally approved: https://discord.com/channels/613813861610684416/1167476806333513800/1237414321445797939 |
👉 Announcement 👈Thank you for the insightful discussions on this FLIP. We feel there is an overwhelming support from the active developer community and the broader ecosystem. On May 15th, 2024 10-11am PT (17-18:00 UTC) we'll hold a joint meeting of the Core Protocol Working Group and the Cadence Language and Execution Working Group, where we would like to briefly go over it together and approve this FLIP unless there are any major concerns remaining. You can add the meeting to your calendar via this link or find it on our 📆 Flow Events & Working Groups calendar along with all the other working group meetings. Everyone is welcome 💚 |
Minor, but would be great to resolve: There are still a few differences between the EVM contract in the implementation and in the FLIP:
@ramtinms @sideninja Could you please update FLIP (more important) and implementation to bring them in sync? |
@turbolent I plan to, after I also complete this one onflow/flow-go#5911 |
Summary of joint Cadence & Execution and Core Protocol Working Groups on May 15, 2024
links to the video recording and meeting transcript here (at the bottom) Next StepsAs there were no concerns in the meeting, we intend to move this FLIP to approved/accepted on Wednesday May 22nd, unless significant concerns are brought up here. Thank you. |
@turbolent I updated all the recent changes in the FLIP. As far as the content goes I believe it should be up to date. |
👉 Announcement 👈
Thank you for the insightful discussions on this FLIP. We feel there is an overwhelming support from the active developer community and the broader ecosystem.
On May 15th, 2024 10-11am PT (17-18:00 UTC) we'll hold a joint meeting of the Core Protocol Working Group and the Cadence Language and Execution Working Group, where we would like to briefly go over it together and approve this FLIP unless there are any major concerns remaining.
You can add the meeting to your calendar via this link or find it on our 📆 Flow Events & Working Groups calendar along with all the other working group meetings. Everyone is welcome 💚