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!: add String support #1042

Merged
merged 20 commits into from
Aug 3, 2023
Merged

Conversation

iqdecay
Copy link
Contributor

@iqdecay iqdecay commented Jul 11, 2023

Note: this PR is dependent on Sway v0.42, and therefore will need #997 before being effective (fuel-core 0.19).

This PR closes #1011 by adding support for Sway's std::string::String type.

  • The ParamType introduced here is called ParamType::StdString because ParamType::String represents statically-sized Sway strings. I think it would be better to use String for the dynamic type, and StaticString for statically-sized ones, but I think this refactoring would make the PR less clear.
  • I need to figure out what's wrong with how I encode the String data, this is why the test is weird.

@iqdecay iqdecay self-assigned this Jul 11, 2023
@iqdecay iqdecay force-pushed the iqdecay/feat-dynamic-string-output branch 2 times, most recently from 2193c94 to bcbbeee Compare July 11, 2023 11:38
@iqdecay iqdecay changed the base branch from master to iqdecay/deps-fuel-core-update July 11, 2023 12:12
Copy link
Contributor

@segfault-magnet segfault-magnet left a comment

Choose a reason for hiding this comment

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

Beautifully done!!

As for the open sway issue, you can test your understanding of the encoding by leaving the SDK out and using a plain ol' sway script calling a sway contract via forc.

Log the received data and do these assertions and see if they hold up.

Not approving yet since the issue might have to do with our encoding (albeit that seems unlikely if String really just uses Bytes under the hood).

@digorithm digorithm added the enhancement New feature or request label Jul 11, 2023
@iqdecay iqdecay force-pushed the iqdecay/feat-dynamic-string-output branch from c61f0e4 to 49a8f42 Compare July 16, 2023 20:01
@iqdecay iqdecay requested a review from IGI-111 July 17, 2023 08:43
Base automatically changed from iqdecay/deps-fuel-core-update to master July 17, 2023 12:04
@iqdecay iqdecay force-pushed the iqdecay/feat-dynamic-string-output branch from 49a8f42 to 831336d Compare July 21, 2023 18:05
@iqdecay
Copy link
Contributor Author

iqdecay commented Jul 21, 2023

Marking as draft until I figure out what is wrong with my encoding.

@iqdecay iqdecay marked this pull request as draft July 21, 2023 18:49
@segfault-magnet segfault-magnet self-assigned this Jul 26, 2023
@segfault-magnet segfault-magnet marked this pull request as ready for review July 26, 2023 18:05
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.

Great work!

@segfault-magnet
Copy link
Contributor

Great work!

Mostly @iqdecay, I added some tests and fixed the encoding bug.

can we please add an issue to track this ?

Adding an issue now.

Copy link
Contributor

@MujkicA MujkicA left a comment

Choose a reason for hiding this comment

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

LGTM

packages/fuels-core/src/codec/abi_encoder.rs Show resolved Hide resolved
packages/fuels-core/src/types/param_types.rs Show resolved Hide resolved
@hal3e hal3e changed the title feat: dynamic string output feat: add String support Aug 3, 2023
@segfault-magnet segfault-magnet changed the title feat: add String support feat!: add String support Aug 3, 2023
@segfault-magnet segfault-magnet merged commit 6f4af1d into master Aug 3, 2023
33 checks passed
@segfault-magnet segfault-magnet deleted the iqdecay/feat-dynamic-string-output branch August 3, 2023 14:22
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.

feat: support String type from sway-libs
6 participants