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.

Sign up for GitHub

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

Jump to bottom

feat: bump forc to 0.46.0 #1148

Merged
merged 18 commits into from
Oct 9, 2023
Merged

feat: bump forc to 0.46.0 #1148

merged 18 commits into from
Oct 9, 2023

Conversation

hal3e
Copy link
Contributor

@hal3e hal3e commented Oct 3, 2023 •
edited
Loading

close: #1139, #1134

I took this over from @segfault-magnet. Most of the work was done by him.

  • AssetId is now mapped to fuels::types::AssetId in abigen!.
  • forc 0.46.0 uses string slices as default and logging them does not work. An error was added to indicate what to do in this case. Hopefully, we can delete this code once Improving slice sway#5110 is resolved.

If this PR is approved, I will create two issues for the TODO items.

Checklist

  • I have linked to any relevant issues.
  • I have updated the documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary labels.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@hal3e hal3e added bug Something isn't working tech-debt Improves code quality or safety labels Oct 3, 2023
@hal3e hal3e self-assigned this Oct 3, 2023
@hal3e hal3e changed the title Feat/asset id abigen feat: bump forc to 0.46.0 Oct 3, 2023
digorithm
digorithm previously approved these changes Oct 3, 2023
View reviewed changes
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.

Amazing work, @hal3e! Please create these follow-up issues when you can.

iqdecay
iqdecay suggested changes Oct 4, 2023
View reviewed changes
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.

just 1 question regarding setup_test_program macro

packages/fuels/tests/types_scripts.rs Show resolved Hide resolved
packages/fuels/tests/logs.rs Outdated Show resolved Hide resolved
@hal3e
Copy link
Contributor Author

hal3e commented Oct 4, 2023

I have create the two new issues: #1152, #1151

iqdecay
iqdecay approved these changes Oct 4, 2023
View reviewed changes
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.

🚢 it

segfault-magnet
segfault-magnet approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@segfault-magnet segfault-magnet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We should probably do two more things (ideally) in this PR:

  1. Have an e2e test for string slices since forc 0.46 effectively puts them in the spotlight
  2. Fail to decode nested string slices as they behave just like raw_slice does. I.e. when they are top-level then the data is inlined, otherwise, you get a ptr,len.

For the second point you might find changes made here to be useful (precisely I mean the wording change from 'is heap type' to 'needs extra receipt' since string slices are heap types, but don't behave like others in the sense we need extra receipts for them.

You can either handle it here or in subsequent tasks. I am approving the PR. Good job :)

Edit:

Don't actually know what our error will be if we nest string slices, I have a feeling due to all the edge cases currently present in master that it might be an unpleasant surprise.

@hal3e
Copy link
Contributor Author

hal3e commented Oct 4, 2023

Good point @segfault-magnet. Let's wait for #1106 to get merged so that I can use the new code. I will then add the tests.

digorithm
digorithm approved these changes Oct 9, 2023
View reviewed changes
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:

@hal3e hal3e merged commit 22f10db into master Oct 9, 2023
39 checks passed
@hal3e hal3e deleted the feat/asset_id_abigen branch October 9, 2023 10:43
@hal3e hal3e mentioned this pull request Oct 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@digorithm digorithm digorithm approved these changes

@Br1ght0ne Br1ght0ne Br1ght0ne approved these changes

@iqdecay iqdecay iqdecay approved these changes

@segfault-magnet segfault-magnet segfault-magnet approved these changes

@MujkicA MujkicA Awaiting requested review from MujkicA MujkicA is a code owner

@Salka1988 Salka1988 Awaiting requested review from Salka1988 Salka1988 is a code owner

Assignees

@hal3e hal3e

Labels
bug Something isn't working tech-debt Improves code quality or safety
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adapt fuels-rs to sway 0.46.0
6 participants
@hal3e @digorithm @Br1ght0ne @iqdecay @segfault-magnet @MujkicA