Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Structs in the ABI #296

Merged
merged 1 commit into from
Mar 11, 2021
Merged

Structs in the ABI #296

merged 1 commit into from
Mar 11, 2021

Conversation

g-r-a-n-t
Copy link
Member

@g-r-a-n-t g-r-a-n-t commented Mar 6, 2021 •
edited
Loading

What was wrong?

fixes #282

Structs were not being added to the ABI. For example, we could not build the JSON ABI for the following example:

struct MyStruct:
  num1: u256
  num2: u256

contract MyContract:
  pub def foo(my_struct: MyStruct) -> MyStruct:
    return my_struct

How was it fixed?

  • Added components to the AbiEncoding trait.
  • Added field names to function and event attributes.
  • Updated the ABI builder to use information from the analyzer.

To-Do

  • OPTIONAL: Update Spec if applicable

  • Add entry to the release notes (may forgo for trivial changes)

  • Clean up commit history

@g-r-a-n-t g-r-a-n-t marked this pull request as draft March 6, 2021 00:43
@g-r-a-n-t g-r-a-n-t changed the title Structs in the ABI ☢️ Structs in the ABI ☢️ Mar 6, 2021
@g-r-a-n-t g-r-a-n-t changed the title ☢️ Structs in the ABI ☢️ ☢️ Structs in the ABI ☢️ Mar 6, 2021
@g-r-a-n-t g-r-a-n-t changed the title ☢️ Structs in the ABI ☢️ Structs in the ABI Mar 9, 2021
g-r-a-n-t
g-r-a-n-t commented Mar 9, 2021
View reviewed changes
analyzer/src/namespace/events.rs Outdated Show resolved Hide resolved
analyzer/src/namespace/scopes.rs Show resolved Hide resolved
analyzer/src/traversal/functions.rs Outdated Show resolved Hide resolved
compiler/src/abi/builder.rs Show resolved Hide resolved
compiler/src/abi/builder.rs Outdated Show resolved Hide resolved
compiler/src/yul/operations/data.rs Outdated Show resolved Hide resolved
compiler/tests/evm_contracts.rs Outdated Show resolved Hide resolved
g-r-a-n-t
g-r-a-n-t commented Mar 10, 2021
View reviewed changes
analyzer/src/lib.rs Outdated Show resolved Hide resolved
analyzer/src/namespace/events.rs Outdated Show resolved Hide resolved
analyzer/src/namespace/types.rs Show resolved Hide resolved
analyzer/src/traversal/expressions.rs Show resolved Hide resolved
compiler/src/abi/elements.rs Show resolved Hide resolved
compiler/src/abi/elements.rs Outdated Show resolved Hide resolved
compiler/tests/fixtures/abi_encoding_stress.fe Outdated Show resolved Hide resolved
@g-r-a-n-t g-r-a-n-t force-pushed the abi-structs branch 4 times, most recently from 5f558cc to ac5736e Compare March 11, 2021 01:28
@g-r-a-n-t g-r-a-n-t marked this pull request as ready for review March 11, 2021 01:46
@g-r-a-n-t g-r-a-n-t requested a review from cburgdorf March 11, 2021 02:39
cburgdorf
cburgdorf approved these changes Mar 11, 2021
View reviewed changes
Copy link
Collaborator

@cburgdorf cburgdorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool!

analyzer/src/lib.rs Show resolved Hide resolved
analyzer/src/lib.rs

/// Attribute contextual information to an event definition node.
pub fn add_event(&mut self, spanned: &Spanned<fe::ContractStmt>, event: EventDef) {
self.events.insert(spanned.span, event);
Copy link
Collaborator

@cburgdorf cburgdorf Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not just related to this change but also to all the other APIs on the context that do self.events.insert(key, value);. I think the fact that we are ignoring the returned Option<T> is a problem. These APIs all assume that the key doesn't exist yet. If for some reason it does exist already we would just silently overwrite it without knowing that we have some underlying issue to fix. We should probably panic here if a value already exists because it means we are dealing with an unrecoverable programming error. If you agree then this would make a good first issues for newcomer to refactor.

analyzer/src/traversal/expressions.rs Show resolved Hide resolved
compiler/src/abi/builder.rs Outdated Show resolved Hide resolved
compiler/tests/fixtures/demos/uniswap.fe
# num3=self.block_time_stamp
# )
pass
pub def get_reserves() -> ThreeNums:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@g-r-a-n-t g-r-a-n-t merged commit ba7bb3a into ethereum:master Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cburgdorf cburgdorf cburgdorf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Structs in json ABI
2 participants
@g-r-a-n-t @cburgdorf