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

Handle ReturnData for other types #161

Merged
merged 3 commits into from
Mar 19, 2022
Merged

Conversation

digorithm
Copy link
Member

Closes #139.

More context in #139. Summary: depending on the size of the returned data, the returned data will live either under the Return receipt or ReturnData receipt. This handles it for the types that weren't being properly handled before.

@digorithm digorithm marked this pull request as ready for review March 18, 2022 22:13
@digorithm digorithm requested a review from iqdecay March 18, 2022 22:14
@digorithm digorithm self-assigned this Mar 18, 2022
@digorithm digorithm added enhancement New feature or request tech-debt Improves code quality or safety labels Mar 18, 2022
Comment on lines +1095 to +1105
let res = contract_instance.get_contract_id().call().await.unwrap();

// First `value` is from `CallResponse`.
// Second `value` is from Sway `ContractId` type.
assert_eq!(
res.value.value,
[
255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255
]
);
Copy link
Member Author

Choose a reason for hiding this comment

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

@nfurfaro I believe this is the relevant case for you: get_contract_id() returns the Sway type ContractId, which wraps the b256 in a Struct, which wasn't being handled before. Now it seems to be working fine!

Comment on lines +52 to 69
match &*self {
// Bits256 Always bigger than one `WORD`.
Self::B256 => true,
Self::String(size) => size > 8,
// Strings are bigger than one `WORD` when its size > 8.
Self::String(size) => size > &8,
Self::Struct(params) => match params.len() {
// If only one component in this struct
// check if this element itself is bigger than a `WORD`.
1 => params[0].bigger_than_word(),
_ => true,
},
// Enums are always in `ReturnData`.
Self::Enum(_params) => true,
// Arrays seem to always be inside `ReturnData`.
Self::Array(_params, _l) => true,
// The other primitive types are inside `Return`,
// thus smaller than one `WORD`.
_ => false,
Copy link
Member Author

Choose a reason for hiding this comment

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

@vnepveu I believe this will be related to your current work on supporting multiple return values, make sure to rebase it on top of this once it's merged!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes will do!

@digorithm digorithm requested a review from nfurfaro March 18, 2022 22:17
@digorithm digorithm changed the title Handle ReturnDatafor other types Handle ReturnData for other types Mar 18, 2022
Copy link

@nfurfaro nfurfaro left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll defer to @vnepveu

},
// Enums are always in `ReturnData`.
Self::Enum(_params) => true,
// Arrays seem to always be inside `ReturnData`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this expected or a bug? cc @otrho

Copy link

Choose a reason for hiding this comment

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

I'm guessing at this stage it's always true since even an enum with a single variant will still have a tag and a value, even if it's just unit. So it can't fit in a register. After we add some gnarly optimisations this may no longer be true, but only if we want to go there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I was pointing at the Arrays seem to always be inside `ReturnData`., not the enums 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately I had to do a lot of guesswork to get this working and figure out where each returned type would be located at.

As for arrays, in the tests I did, they were always in ReturnData (there are few new array-as-output tests added in this PR). But then again, I didn't fuzz or anything, just tested a few cases and focused on solving some existing blockers.

But, intuitively (which could be wrong of course), if any primitive type takes a whole word. Even if it doesn't fill it all we pad it. So an array should be larger than a word? Maybe unless is an array of size zero or one? 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

An array of size 0 or 1 should only take up a single word. Could you add a test for that @digorithm?

Copy link
Contributor

@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.

LGTM

Comment on lines +52 to 69
match &*self {
// Bits256 Always bigger than one `WORD`.
Self::B256 => true,
Self::String(size) => size > 8,
// Strings are bigger than one `WORD` when its size > 8.
Self::String(size) => size > &8,
Self::Struct(params) => match params.len() {
// If only one component in this struct
// check if this element itself is bigger than a `WORD`.
1 => params[0].bigger_than_word(),
_ => true,
},
// Enums are always in `ReturnData`.
Self::Enum(_params) => true,
// Arrays seem to always be inside `ReturnData`.
Self::Array(_params, _l) => true,
// The other primitive types are inside `Return`,
// thus smaller than one `WORD`.
_ => false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes will do!

Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Can handle arrays of size 0 or 1 in a future PR. Make sure there's an issue for it.

@digorithm
Copy link
Member Author

Tracking it here: #162.

@digorithm digorithm merged commit 94ff8f7 into master Mar 19, 2022
@digorithm digorithm deleted the rodrigo/large-return-handling branch March 20, 2022 00:00
@digorithm digorithm mentioned this pull request Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request tech-debt Improves code quality or safety
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Extend data handling for types larger than a WORD
6 participants