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

Bump to fuels 0.17.0 #2273

Merged
merged 6 commits into from
Jul 8, 2022
Merged

Conversation

mohammadfawaz
Copy link
Contributor

@mohammadfawaz mohammadfawaz commented Jul 8, 2022

  • Bump SDK tests to fuels 0.17.0
    • one test case is failing that I'll look into. Otherwise, everything looks good.
  • Bump forc new to use 0.17.0

)),
)
.await
.unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this locally with a reasonable project name and ran cargo fmt and it did nothing which means that the output of this is already well formatted.

@@ -251,7 +254,7 @@ async fn can_get_tx_output_type() {
// Contract output
let output_type = 1;
let result_ptr = contract_instance
.get_tx_output_pointer(1)
.get_tx_output_pointer(0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@digorithm is this change expected? Are the outputs added in a different order now?

Copy link
Member

Choose a reason for hiding this comment

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

We did touch on some parts related to that in recent changes, but I'm not sure about the context here -- what's this test doing? What's the expected behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This methods basically calls the following standard library function: https://github.com/FuelLabs/sway/blob/master/sway-lib-std/src/tx.sw#L277-L283.
It just looks like the tx output at index 0 is now of "type 1" and the tx output at index 1 is of "type 3". Previously, they were in the opposite order, that's all.

@mohammadfawaz mohammadfawaz marked this pull request as ready for review July 8, 2022 15:46
@mohammadfawaz mohammadfawaz requested review from nfurfaro, adlerjohn, digorithm and a team July 8, 2022 15:47
@mohammadfawaz mohammadfawaz self-assigned this Jul 8, 2022
@mohammadfawaz mohammadfawaz added forc testing General testing labels Jul 8, 2022
@mohammadfawaz mohammadfawaz mentioned this pull request Jul 8, 2022
4 tasks
Copy link
Contributor

@sezna sezna 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 will defer to @digorithm for the above comment

@mohammadfawaz mohammadfawaz requested a review from a team July 8, 2022 19:51
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.

After discussing this with @segfault-magnet, yes, the change related to Outputs was introduced in the fuels-rs's Multicall PR. Here's what @segfault-magnet said:

originally the first output we'd insert was the asset change one:
https://github.com/FuelLabs/fuels-rs/pull/399/files#diff-436dedff71aee8cbbb04812b7dd33fd33ee52ea2fccb5d2d83ea8bba68d2e34eL77

then we would do the outputs for the external contracts:
https://github.com/FuelLabs/fuels-rs/pull/399/files#diff-436dedff71aee8cbbb04812b7dd33fd33ee52ea2fccb5d2d83ea8bba68d2e34eL135

now we first do the outputs for the external contracts:
https://github.com/FuelLabs/fuels-rs/pull/399/files#diff-436dedff71aee8cbbb04812b7dd33fd33ee52ea2fccb5d2d83ea8bba68d2e34eR218

and then the asset change
https://github.com/FuelLabs/fuels-rs/pull/399/files#diff-436dedff71aee8cbbb04812b7dd33fd33ee52ea2fccb5d2d83ea8bba68d2e34eR241

The external contracts ids are now stored in a hashset, while they were initially in a vec, so if there ever was an ordering with the vec, now there definitely isn't one with HashSet. 

So, yes, the order of asset change and outputs for external contracts was flipped.

@mohammadfawaz mohammadfawaz enabled auto-merge (squash) July 8, 2022 21:30
@mohammadfawaz mohammadfawaz merged commit 5f95c72 into master Jul 8, 2022
@mohammadfawaz mohammadfawaz deleted the mohammadfawaz/bump_to_fuels_0_17_0 branch July 8, 2022 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forc testing General testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants