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: vector input support #597

Merged
merged 103 commits into from
Oct 3, 2022
Merged

Conversation

segfault-magnet
Copy link
Contributor

@segfault-magnet segfault-magnet commented Sep 27, 2022

closes: #353

Vectors in function outputs are currently not supported. Waiting on compiler support.

Most notable changes:

  1. Unresolved Bytes
    Encoding something via the ABIEncoder will no longer give you raw bytes right away. You get something called UnresolvedBytes. Think of an unresolved symbol during linking :D

If you give UnresolvedBytes the address at which the bytes are going to be placed, then it will reward you with the bytes themselves. This is so that we may resolve any pointers (currently only vectors) within the data.

2. Reverse decoding
Returning a vector involves logging its elements. We need to know where those logs are in, what could be, a sea of other unrelated logs. We cannot require the vec element logs to be the first thing logged since that would mean whatever you do to calculate the vector must not log.

The only reasonable approach to me was to say that the last logs of the method must be the vector elements themselves.

So that means reverse decoding since you don't know from ParamTypes how big the vector is going to be. So you kind of decode in reverse until everyone is happy :) ~

  1. Decoding now requires the ReturnData as well as Logs to decode.

To generate methods that actually return vectors, we need the sway contract methods themselves to return vectors.

But returning a vector from sway gives you very little to work with. Only 3 WORDs, the pointer, the capacity and the length of the vector you're returning.

The rest of the information should be logged rather than returned. Until further notice that burden falls on the authors of the contracts.

There is a caveat to logging in sway as well -- if you log a vector you're not going to be logging its elements. Just like when you return a vector -- you'll only get those 3 WORDS.

This results in an edge case where if you have a Vec<Vec<..>> you need to jump through an extra hoop to have it decode properly. This is due to log not penetrating the Vec iron curtain, so if you log the parent vec you're going to just get it's ptr, cap and len and not those of its children elements.

That is why you need to log the inner vectors themselves after you log their elements. This is because we're decoding from the right (aka reverse) and we need to get the vec length info before we get the elements.

And why we're decoding in reverse is explained somewhere above.

There is a sync shortly regarding vectors, hopefully, the compiler team can figure something out to either make this logging automatic or provide a recursive helper which can penetrate the veil of a vector :) This behavior is documented in the SDK book.

Co-authored-by: Ahmed Mujkic <32431923+MujkicA@users.noreply.github.com>
Copy link
Contributor

@hal3e hal3e left a comment

Choose a reason for hiding this comment

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

Nice work! This is my first review pass. I have to rest a bit and try to wrap my head around the resolve thing 😄

docs/src/types/vectors.md Outdated Show resolved Hide resolved
docs/src/types/vectors.md Outdated Show resolved Hide resolved
packages/fuels-core/src/abi_encoder.rs Outdated Show resolved Hide resolved
packages/fuels/tests/harness.rs Outdated Show resolved Hide resolved
@segfault-magnet
Copy link
Contributor Author

segfault-magnet commented Sep 29, 2022

Nice work! This is my first review pass. I have to rest a bit and try to wrap my head around the resolve thing smile

Ty.
I'm glad you said that. This means that I probably need to put some comments in.

hal3e
hal3e previously approved these changes Sep 29, 2022
Copy link
Contributor

@hal3e hal3e left a comment

Choose a reason for hiding this comment

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

Thanks for adding the comments. It is much easier now to understand what is happening where. LGTM! Good work! 🎉

Salka1988
Salka1988 previously approved these changes Sep 29, 2022
Copy link
Member

@Salka1988 Salka1988 left a comment

Choose a reason for hiding this comment

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

As Hal3e said, thanks for the explanation! LGTM 👍

@segfault-magnet segfault-magnet changed the title feat: vector input/output support feat: vector input support Sep 29, 2022
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.

Wonderful work. Loved the refactors and abstraction you introduced you did along the way.

Left a micro nit. Also, sync with master when possible!

packages/fuels-core/src/abi_encoder.rs Show resolved Hide resolved
// "RawVec" is part of the Vec structure. Not used in the SDK and thus
// not generated.
Ok(
["ContractId", "Address", "Option", "Result", "Vec", "RawVec"]
Copy link
Member

Choose a reason for hiding this comment

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

These should still be consts extracted away from this function, though. Easier to maintain/find/refer to. Plus, there's a new "Identity" one in master; you'll see it once you sync with it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully, I can sway you with the following :D

These should still be consts

Godbolt shows both let a = "something" and const a: &str = "something" results in the same asm.

extracted away from this function

They have meaning only in this function. I'd rather not place them somewhere else.

Easier to maintain/find/refer to.

They are inside a 10line function. Only to be used here. The keywords are used only once and in such a way that their usage gives them meaning -- i.e. if my type matches any of the following ones.

there's a new "Identity" one in master; you'll see it once you sync with it!

I've added it just now :)

Copy link
Member

Choose a reason for hiding this comment

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

Godbolt shows both let a = "something" and const a: &str = "something" results in the same asm.

My suggestion is really not related to the generated asm 😂. 100% related to readability. But this:

They have meaning only in this function. I'd rather not place them somewhere else.

Makes it ok for it to be inside this function (and, thus, not a const). The moment we start referencing this from other places, then we should extract it out. But yeah, not needed right now!

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.

:shipit:

@segfault-magnet segfault-magnet merged commit 450e9ae into master Oct 3, 2022
@segfault-magnet segfault-magnet deleted the segfault-magnet/vec_support branch October 3, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vec and heap type support
6 participants