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.

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

feat: support () type in typegen #2122

Merged
merged 16 commits into from Apr 23, 2024

Conversation

nedsalk
Copy link
Contributor

@nedsalk nedsalk commented Apr 19, 2024

While implementing #2070 I came across an issue with call-test-contract's foobar function that takes in an empty () type. Typegen couldn't handle it and was throwing so I implemented the logic for supporting () types.
Now, empty inputs are ignored in typegen. This corresponds to how we're encoding empty arguments during runtime - we ignore them. I added tests to verify this runtime behavior as well.

Co-authored-by: Peter Smith <peter@blueoceancomputing.co.uk>
@arboleya
Copy link
Member

Now, functions that take in an empty type mandate passing undefined into them:

const fn = contract.functions.foobar(undefined); // InvokeFunction<[value: undefined], ...>

Oof. It's a tough one - I'm not sure I can handle it. 😅

@nedsalk

This comment was marked as outdated.

@nedsalk nedsalk marked this pull request as draft April 19, 2024 14:01
@nedsalk

This comment was marked as outdated.

@nedsalk nedsalk marked this pull request as ready for review April 23, 2024 07:45
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

These feel ok:

contract.functions.empty().call();
contract.functions.value_then_empty(35).call();

These do not:

contract.functions.empty_then_value(35).call();
contract.functions.value_then_empty_then_value(35, 35).call();

I believe the expected usage would be somewhat similar to the following:

contract.functions.empty_then_value(null, 35).call();
contract.functions.value_then_empty_then_value(35, null, 35).call();

EDIT: If empty types are always ignored, should they exist in Sway and compile successfully? Do you know their purpose?

@nedsalk
Copy link
Contributor Author

nedsalk commented Apr 23, 2024

@arboleya I don't know the purpose of the empty types.

I agree with you that the expected behavior should be the one you mentioned for all cases. I didn't implement it like that because it required changes to the runtime behavior of the ABI coder, because currently the abi-coder also removes the empty types and fails if you provide it a value like in your proposal.
I didn't go down that route because of the critical work going on with the new ABI encoding. These edge case runtime tests I added will have to be green for the new encoding as well and maybe we'll have to change the runtime behavior for empty types due to them. Thus, IMO we should handle these oddities in a separate PR once the dust settles. What do you think?

@nedsalk nedsalk requested a review from arboleya April 23, 2024 14:10
arboleya
arboleya previously approved these changes Apr 23, 2024
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

Please create a detailed follow-up issue with the adjustments you mentioned on the ABI Coder.

Using our tests to dogfood Typegen was a nice touch! 👏 🚀

I hope this is also beneficial for browser testing.

Co-authored-by: Anderson Arboleya <anderson@arboleya.me>
@nedsalk nedsalk requested a review from arboleya April 23, 2024 14:44
Copy link
Contributor

@petertonysmith94 petertonysmith94 left a comment

Choose a reason for hiding this comment

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

Looks wonderful 🚢

@nedsalk nedsalk enabled auto-merge (squash) April 23, 2024 16:51
Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.34%(+0.02%) 69.3%(+0%) 77.38%(+0.06%) 79.49%(+0.02%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
✨ packages/abi-typegen/src/abi/types/EmptyType.ts 100%
(+100%)
100%
(+100%)
100%
(+100%)
100%
(+100%)

@nedsalk nedsalk merged commit 1542cdc into master Apr 23, 2024
17 checks passed
@nedsalk nedsalk deleted the ns/feat/support-empty-sway-type-in-typegen branch April 23, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants