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

feat: support Tuple in Enum and Enum in Tuple #213

Merged
merged 22 commits into from
Apr 27, 2022
Merged

feat: support Tuple in Enum and Enum in Tuple #213

merged 22 commits into from
Apr 27, 2022

Conversation

iqdecay
Copy link
Contributor

@iqdecay iqdecay commented Apr 14, 2022 •
edited
Loading

This PR closes #53 by supporting nested struct and enums as return types.
It also closes #63 because of refactoring.
Some caveats:

  • Currently does not work as inputs, but the infrastructure is theoritically here. I'm waiting for the RESERV00 bug to be solved to see if it works
  • Arbitrary depth of nesting has not been tested yet!

@iqdecay iqdecay requested a review from digorithm as a code owner April 14, 2022 13:50
@iqdecay iqdecay self-assigned this Apr 14, 2022
@iqdecay iqdecay marked this pull request as draft April 14, 2022 13:50
@iqdecay iqdecay added the enhancement New feature or request label Apr 14, 2022
@iqdecay iqdecay force-pushed the vnepveu/feat-enum-in-structs branch 5 times, most recently from ef31d32 to e00f4de Compare April 20, 2022 20:30
@iqdecay
fix: fix the string expansion of ParamType::Enum
4ea8f89
@iqdecay
refactor: rename the function for finding custom types in the ABI
20727da
@iqdecay
fix: consider both types in nested custom types
e2779f4 
* Previously, inside a `struct` we were considering only `struct` types,
and inside `enum` we were considering only `enum` types. We know look
for either.
@iqdecay
refactor: rename function for expanding enums and structs
d85de57
@iqdecay
fix: don't derive Default trait on nested Enum types
5ad0a22
@iqdecay
feat: support Enum inside Struct in ONE example
2c9d750 
* run `cargo t workflow_enum_inside_struct` with all other tests from
  the harness commented-out (LOL)
@iqdecay
feature: use proper macro expansion for Enum::new_from_tokens methods
42b43ae
@iqdecay
test: improve get_custom_types naming and testing
6349656
@iqdecay
test: add test for Struct inside Enum before implementation
9abae48
@iqdecay
feat: expand Struct inside Enum
1d348ca 
- The code at this point expands correctly, and the test runs
@iqdecay
test: simplify first test for struct_inside_enum
95284cc
@iqdecay
test: add unit test for the expansion of Struct inside Enum
5e12f92
@iqdecay iqdecay force-pushed the vnepveu/feat-enum-in-structs branch from 57de094 to 5e12f92 Compare April 21, 2022 14:21
@iqdecay
test: add compiling but non-working test
78e41d0 
* The test fails with `Expected `Tuple` found `Enum(xxx)`
@digorithm
Copy link
Member

Moving this summary of our conversation here so it's explicit what needs to be done:

  1. We need to support enum output by updating expand_fn_outputs to include the enum name instead of its components, which looks like a tuple, leading to the wrong Tokenizable;
  2. We need to abigen Tokenizable for enum types.

@iqdecay iqdecay marked this pull request as ready for review April 23, 2022 16:23
@iqdecay iqdecay requested a review from adlerjohn April 23, 2022 16:35
@iqdecay iqdecay requested a review from m3talsmith April 23, 2022 16:35
iqdecay
iqdecay commented Apr 25, 2022
View reviewed changes
Copy link
Contributor Author

@iqdecay iqdecay left a comment

Choose a reason for hiding this comment

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

Just re-reading what I did, it's way too big lol

packages/fuels-abigen-macro/tests/test_projects/enum_inside_struct/Forc.toml Outdated Show resolved Hide resolved
packages/fuels-abigen-macro/tests/harness.rs Outdated Show resolved Hide resolved
packages/fuels-core/src/json_abi.rs Outdated Show resolved Hide resolved
@digorithm
Copy link
Member

@vnepveu Is this still a draft? Or officially ready for review? Let me know so I can take another deep pass.

@iqdecay
Copy link
Contributor Author

iqdecay commented Apr 25, 2022

It's definitely ready for review just thought I'd review myself as well

digorithm
digorithm requested changes Apr 26, 2022
View reviewed changes
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

It's looking really good. Great job on this -- this is no simple feat 😃. Just left some comments and improvement suggestions.

@iqdecay iqdecay mentioned this pull request Apr 27, 2022
Closed
@iqdecay iqdecay merged commit caedb6c into master Apr 27, 2022
@iqdecay iqdecay deleted the vnepveu/feat-enum-in-structs branch April 27, 2022 17:23
@iqdecay iqdecay mentioned this pull request May 12, 2022
Merged
iqdecay added a commit that referenced this pull request May 12, 2022
@iqdecay
test: test support for nested custom types in function arguments (#276)
2b335d7 
This PR definitely closes #53 by testing support for Struct inside Enum and Enum
inside Struct types as function arguments in smart contracts.
* It was only supported as function outputs since #213.I
* In #275 support was added for the Enum type as function argument, and it turns out that nested custom types as function argument worked right-away with this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@digorithm digorithm digorithm approved these changes

@m3talsmith m3talsmith Awaiting requested review from m3talsmith

@adlerjohn adlerjohn Awaiting requested review from adlerjohn

Assignees

@iqdecay iqdecay

Labels
enhancement New feature or request
Projects
Fuel Network
Archived in project
Milestone
No milestone
3 participants
@iqdecay @digorithm @adlerjohn