ABI refactor and removal of bytes #472

g-r-a-n-t · 2021-06-29T17:11:24Z

What was wrong?

The original ABI encoding/decoding implementation made it difficult to do two things:

insert runtime checks
add support for recursive encodings

How was it fixed?

insert runtime checks

These checks have not been added yet, but this makes it easier to add them.

add support for recursive encodings

Also not implemented, but I think it should be somewhat easier now.

Some other changes:

got rid of the bytes type. u8[n] is now encoded as a dynamically sized bytes array.
made yulc panics readable
changed the type of msg.sig to u256

To-Do

OPTIONAL: Update Spec if applicable
Add entry to the release notes (may forgo for trivial changes)
Clean up commit history

analyzer/src/namespace/types.rs

compiler/src/yul/constructor.rs

compiler/src/yul/runtime/abi_dispatcher.rs

compiler/src/yul/runtime/functions/abi.rs

codecov-commenter · 2021-06-30T05:32:07Z

Codecov Report

Merging #472 (9dfb446) into master (ff5df97) will increase coverage by 0.00%.
The diff coverage is 97.77%.

❗ Current head 9dfb446 differs from pull request most recent head 9af614a. Consider uploading reports for the commit 9af614a to get more accurate results

@@           Coverage Diff           @@
##           master     #472   +/-   ##
=======================================
  Coverage   87.31%   87.32%           
=======================================
  Files          76       77    +1     
  Lines        5046     5081   +35     
=======================================
+ Hits         4406     4437   +31     
- Misses        640      644    +4

Impacted Files	Coverage Δ
crates/analyzer/src/traversal/types.rs	`100.00% <ø> (ø)`
crates/driver/src/lib.rs	`75.67% <0.00%> (-4.33%)`	⬇️
crates/lowering/src/names.rs	`78.57% <ø> (ø)`
crates/yulgen/src/lib.rs	`100.00% <ø> (ø)`
crates/yulgen/src/mappers/contracts.rs	`100.00% <ø> (ø)`
crates/yulgen/src/mappers/expressions.rs	`90.00% <ø> (ø)`
crates/yulgen/src/mappers/functions.rs	`97.50% <ø> (ø)`
crates/yulgen/src/utils.rs	`100.00% <ø> (+6.89%)`	⬆️
crates/analyzer/src/namespace/types.rs	`81.06% <91.17%> (-1.85%)`	⬇️
crates/yulgen/src/names/abi.rs	`98.64% <98.64%> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff5df97...9af614a. Read the comment docs.

crates/analyzer/src/namespace/types.rs

crates/yulgen/src/operations/abi.rs

crates/analyzer/src/namespace/types.rs

crates/yulgen/src/runtime/functions/abi.rs

crates/yulgen/src/runtime/functions/contracts.rs

crates/yulgen/src/utils.rs

g-r-a-n-t · 2021-06-30T16:47:07Z

Here's a sample of the new yulc panic formatting:

Error: DeclarationError: Identifier "params_start_code" not found.
 --> input.yul:5:49:
  |
5 |         let params_size := sub(params_end_code, params_start_code)
  |                                                 ^^^^^^^^^^^^^^^^^


Error: DeclarationError: Identifier "params_start_code" not found.
 --> input.yul:8:36:
  |
8 |         codecopy(params_start_mem, params_start_code, params_size)
  |                                    ^^^^^^^^^^^^^^^^^


thread 'main' panicked at 'Yul compilation failed with the above errors', crates/driver/src/lib.rs:86:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

You've hit an internal compiler error. This is a bug in the Fe compiler.
Fe is still under heavy development, and isn't yet ready for production use.

If you would, please report this bug at the following URL:
  https://github.com/ethereum/fe/issues/new

crates/yulgen/src/names/abi.rs

crates/yulgen/src/runtime/functions/abi.rs

g-r-a-n-t · 2021-07-01T02:15:31Z

crates/yulgen/tests/snapshots/yulgen__abi_decode_component_address_mem_function.snap

@@ -0,0 +1,9 @@
+---


the snapshots are worth looking at

g-r-a-n-t · 2021-07-02T00:41:23Z

crates/yulgen/src/runtime/functions/contracts.rs

-                    vec![function.return_type],
+                let decoding_operation = abi_operations::decode_data(
+                    &[function.return_type],
+                    identifier_expression! { outstart },
                    identifier_expression! { outstart },


this should be outstart + outsize. it's not an issue rn since the data_end parameter is ignored, so Im just going to fix it in #440

cburgdorf

I just glanced over it but seems fine to me :)

crates/analyzer/src/namespace/types.rs

sbillig

👍

sbillig · 2021-07-02T17:33:43Z

crates/yulgen/src/names/abi.rs

+        AbiType::Bool => "bool".to_string(),
+        AbiType::Address => "address".to_string(),
+        AbiType::StaticArray { size, inner } => format!("array_{}_{}", size, typ(inner)),
+        AbiType::Tuple { components } => format!("tuple_{}", types(components)),


This will presumably allow for naming collisions when we support nested tuples and structs. Doesn't necessarily have to be addressed now, but we need standard method for collision-avoidance everywhere code is generated (as we discussed yesterday). In the lowering pass, I added a trailing underscore to the tuple name, but maybe we should just add an incrementing number to every generated identifier (eg add a $N suffix, which I think @Y-Nak suggested), rather than trying to differentiate between different nestings by adding a character that stands in for the parentheses.
eg
(A, (B, C), D) -> abi_decode_component_tuple_A_tuple_B_C_D$0
(A, (B, C, D)) -> abi_decode_component_tuple_A_tuple_B_C_D$1

g-r-a-n-t commented Jun 29, 2021

View reviewed changes

g-r-a-n-t force-pushed the abi-refactor-and-removal-of-bytes branch 2 times, most recently from 43907c4 to 0403161 Compare June 30, 2021 05:03

g-r-a-n-t commented Jun 30, 2021

View reviewed changes

crates/yulgen/src/names/abi.rs Outdated Show resolved Hide resolved

crates/yulgen/src/runtime/functions/abi.rs Outdated Show resolved Hide resolved

g-r-a-n-t force-pushed the abi-refactor-and-removal-of-bytes branch from 381e37f to ce1db94 Compare July 1, 2021 02:13

g-r-a-n-t commented Jul 1, 2021

View reviewed changes

g-r-a-n-t force-pushed the abi-refactor-and-removal-of-bytes branch from ce1db94 to 6e4eefc Compare July 1, 2021 02:18

g-r-a-n-t requested review from sbillig and cburgdorf July 1, 2021 03:02

g-r-a-n-t commented Jul 2, 2021

View reviewed changes

cburgdorf approved these changes Jul 2, 2021

View reviewed changes

crates/analyzer/src/namespace/types.rs Outdated Show resolved Hide resolved

sbillig approved these changes Jul 2, 2021

View reviewed changes

ABI encoding refactor, removal of bytes, other misc yulgen changes.

9af614a

g-r-a-n-t force-pushed the abi-refactor-and-removal-of-bytes branch from 6e4eefc to 9af614a Compare July 2, 2021 20:21

g-r-a-n-t merged commit 618f267 into ethereum:master Jul 2, 2021

g-r-a-n-t mentioned this pull request Jul 5, 2021

Use a sound naming strategy when generating identifiers #477

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ABI refactor and removal of bytes #472

ABI refactor and removal of bytes #472

g-r-a-n-t commented Jun 29, 2021 •

edited

Loading

codecov-commenter commented Jun 30, 2021 •

edited

Loading

g-r-a-n-t commented Jun 30, 2021

g-r-a-n-t Jul 1, 2021

g-r-a-n-t Jul 2, 2021

cburgdorf left a comment

sbillig left a comment

sbillig Jul 2, 2021

ABI refactor and removal of bytes #472

ABI refactor and removal of bytes #472

Conversation

g-r-a-n-t commented Jun 29, 2021 • edited Loading

What was wrong?

How was it fixed?

To-Do

codecov-commenter commented Jun 30, 2021 • edited Loading

Codecov Report

g-r-a-n-t commented Jun 30, 2021

g-r-a-n-t Jul 1, 2021

Choose a reason for hiding this comment

g-r-a-n-t Jul 2, 2021

Choose a reason for hiding this comment

cburgdorf left a comment

Choose a reason for hiding this comment

sbillig left a comment

Choose a reason for hiding this comment

sbillig Jul 2, 2021

Choose a reason for hiding this comment

g-r-a-n-t commented Jun 29, 2021 •

edited

Loading

codecov-commenter commented Jun 30, 2021 •

edited

Loading