-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Provide EVM support for some linera features #3847
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
base: main
Are you sure you want to change the base?
Provide EVM support for some linera features #3847
Conversation
linera-execution/src/evm/revm.rs
Outdated
} | ||
|
||
impl PrecompileTag { | ||
fn from_u8(tag: u8) -> Result<Self, String> { |
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.
Is there a way we could avoid this boilerplate?
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.
(For instance using bincode
or bcs
serialization)
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.
Yes, there are ways, but they are not necessarily as simple as 10 lines.
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.
Sure. It's also a question of coding style: We consistently try to avoid boilerplate in the rest of the codebase
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.
Ok, changed.
|
||
struct OptionU32 { | ||
bool has_value; | ||
uint32 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.
Is the code in this file generated or hand-written?
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.
Partially computer generated, partially hand written.
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.
Could we split them then? Manually-written from the generated ones and tag the generated file with an appropriate notice in the header about:
- how it was generated (command)
- when it was generated (timestamp)
- that it MUST NOT be modified manually (only via fixing the source generator)
?
Otherwise it's impossible to tell what should be generated and what has to be manually implemented.
For example, I don't know if I should review the functions that (de)serialize like bcs does b/c those should be generated.
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.
Many changes were made to the generated code:
- I changed some types which were cumbersome and replaced them with "bytes32" or similar. And I want to change appropriately the
serde-reflection
. - I also added checks for the nature of the object for boolean and also this will go into
serde-reflection
. - I removed the functions that are unused.
- I reformatted the generated code with "forge fmt"
- Some types like
MessageIsBouncing
are a simpler version than the generated code. - I avoided having
CryptoHash
and instead put directly "bytes32" - Some name like "function chain_id" causes a namespace collision with other "chain_id" in the code that had to be renamed.
So, I am not sure how to document all that.
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'd say that all of of the changes that you made to the generated code should be made to the generator itself. We should not modify the generated code.
If you feel like there needs to be some "translation layer"/adapter between the generated code and this codebase then it should be added in addition to the generated one AND in a separate file, with proper comments explaining why the generated output is not sufficient.
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.
Well, we need to have some contextual changes. How is the generated code supposed to know that we have a function chain_id()
, and so that there is a namespace collision?
How is the serde-generate
supposed to know which functions are used?
Otherwise, yes I agree that some of the changes will find its way into serde-generate
. But that is not immediate.
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 didn't mean that the Linera API should end up in the generated code but all the mappings between Solidity<>Linera serialization could and 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.
In the end, the computer-generated types are not going to look great. Where is the harm in writing specific types?
Motivation
The Linera features need to be provided for EVM. Some of this is done here.
Proposal
The following is provided:
The precompile is extended for that purpose and the handling of the various cases is reorganized for clarity.
Test Plan
A test for the features has been provided in the CI.
Release Plan
The goal is to provide all this for TestNet 3.
Links
None.