-
Notifications
You must be signed in to change notification settings - Fork 187
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
Types cleanup #485
Types cleanup #485
Conversation
6edbd31
to
7849b1e
Compare
if let Type::Struct(val) = &error_attributes.typ { | ||
context.revert_errors.insert(val.clone()); | ||
if let Type::Struct(_struct) = &error_attributes.typ { | ||
context.revert_errors.insert(_struct.clone()); |
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.
note how we can get rid of this
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.
Instead of adding this information to the context and then building a list of Yul functions based on it, I think we should accumulate functions as operations are used. This should make things more manageable.
Basically, inside of the operation function, we would have a line like context.add_deps(operation_deps)
, where operation_deps
is a list functions needed in the Yul runtime for the operation to work.
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, that sounds like a good idea 👍
|
||
let revert_data = expressions::expr(context, error_expr); | ||
let size = abi_operations::encoding_size(&[val.clone()], vec![revert_data.clone()]); |
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.
encoding size is found in the revert function
49dc4bc
to
1e53e88
Compare
Codecov Report
@@ Coverage Diff @@
## master #485 +/- ##
==========================================
+ Coverage 87.70% 87.79% +0.08%
==========================================
Files 78 80 +2
Lines 5263 5268 +5
==========================================
+ Hits 4616 4625 +9
+ Misses 647 643 -4
Continue to review full report at Codecov.
|
if (iszero([test])) { | ||
([revert_fn]([msg], [size])) | ||
[revert_operations::error_revert(&AbiType::from(string), msg)] |
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.
This looks a whole lot cleaner indeed :)
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.
Looks solid 👍
1e53e88
to
3d2fb1a
Compare
What was wrong?
All type traits were defined in the analyzer crate, when they should have been split up among the crates where they're actually used.
How was it fixed?
AbiEncoding
trait and used a new trait calledJsonAbi
in the abi crate.From<..>
onAbiType
for each Fe type.FeSized
trait was renamed toEvmSized
and moved to the yulgen crate.There were a few other things done in this PR:
To-Do
OPTIONAL: Update Spec if applicable
Add entry to the release notes (may forgo for trivial changes)
Clean up commit history