-
Notifications
You must be signed in to change notification settings - Fork 27
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
Provide support for solidity in serde_generate
#61
base: main
Are you sure you want to change the base?
Provide support for solidity in serde_generate
#61
Conversation
serde-generate/src/solidity.rs
Outdated
} | ||
} | ||
|
||
fn need_memory(sol_format: &SolFormat, sol_registry: &SolRegistry) -> bool { |
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.
Can we make that a method of either SolRegistry
or SolFormat
?
} | ||
} | ||
|
||
pub fn output<T: std::io::Write>(&self, out: &mut IndentedWriter<T>) -> Result<()> { |
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.
Probably ok for a first version but I'm assuming re-using a library would cheaper in terms of gas?
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 the opposite:
- When we have all the source code in one single file, then the LLVM intermediate representation is fully available therefore optimizations are available.
- When a library is available, then the linking occurs when the library is deployed. This of course prevents some optimization. I think it has its uses if the library is an external one. However, we do not gain anything by having a library.
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 can add an option later.
#62 should help with CI. Let's see if it comes back green. |
f1c98ad
to
3dec9d1
Compare
This reverts commit 91adf02.
e25765b
to
a2b5f4a
Compare
generator.output(&mut test_file, ®istry).unwrap(); | ||
} | ||
|
||
let _bytecode = get_bytecode(path, "test.sol", "test").unwrap(); |
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.
let _bytecode =
is not necessarily (and slightly counter-productive. E.g. it disables the warning that would be triggered if you forget the .unwrap()
here).
let ExecutionResult::Success { | ||
reason: _, | ||
gas_used: _, | ||
gas_refunded: _, | ||
logs: _, | ||
output, | ||
} = result |
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.
let ExecutionResult::Success { output, .. } = result
is probably fine, especially for testing.
output: _, | ||
} = result | ||
else { | ||
panic!("The TxKind::Call execution failed to be done"); |
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.
remove "to be done"
serde-generate/src/solidity.rs
Outdated
impl SolRegistry { | ||
fn insert(&mut self, sol_format: SolFormat) { | ||
let key_name = sol_format.key_name(); | ||
if matches!(sol_format, SolFormat::Primitive(Primitive::I8)) { |
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 cases look disjoint. Why not match sol_format { .. }
?
_ => {} | ||
} | ||
// Typename entries are by definition already covered and do not need | ||
// to be inserted. Others do. | ||
if !matches!(sol_format, SolFormat::TypeName(_)) { | ||
self.names.insert(key_name, sol_format); | ||
} |
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.
There's really no need for matches!
here.
SolFormat::Primitive(SolFormat::TypeName(_)) => {
// Typename entries do not need to be inserted.
}
_ => {
self.names.insert(key_name, sol_format);
}
Summary
Solidity is the Javascript of blockchains, therefore having a way to support solidity in one of the codes is quite important.
Solidity has a number of limitations that force some adaptations:
There are also some more mundane restrictions where we choose to simply panic with an explicit error if this occurs in the input code:
The main implication of the use of struct to represent nested structure is that it breaks the design of
ContainerFormat
/Format
. So, we need to introduce the recursive typeSolFormat
type that provides support for this.Other design choices:
get_keywords
is a standard function instead of being a static one as in the OCaml generator. I think it does not matter.abi.encode
/abi.decode
that works with padded serialization. It has anabi.encodePacked
that provides a compact serialization, but it has noabi.decodePacked
. Added to that the BCS serialization of 42_u32 is [42,0,0,0] but the solidity serialization is [0,0,0,42]. Therefore hand-crafted code has been written for processing all primitive types.Test Plan
The code is tested by running an EVM virtual machine. This is nicely provided by the REVM crate.
The only external files used are the temporary ones created during the test which makes the development smooth.