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!: remove legacy encoding #1338

Merged
merged 28 commits into from
May 7, 2024
Merged

feat!: remove legacy encoding #1338

merged 28 commits into from
May 7, 2024

Conversation

hal3e
Copy link
Contributor

@hal3e hal3e commented Apr 18, 2024

closes: #1318

This PR removes all code related to the legacy encoding.

Blocked by configurables not supporting new encoding. Issue to track: FuelLabs/sway#5727

BREAKING CHANGE:

  • Rename resolve_fn_selector to encode_fn_selector

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 big breaking Introduces or requires breaking changes blocked labels Apr 18, 2024
@hal3e hal3e self-assigned this Apr 18, 2024
@MujkicA MujkicA mentioned this pull request Apr 24, 2024
6 tasks
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.

So configurables are broken until the sway issue is fixed? Do we plan on releasing this before the fix for whatever cyclic-dep reason?

packages/fuels-accounts/src/predicate.rs Outdated Show resolved Hide resolved
packages/fuels/Forc.toml Outdated Show resolved Hide resolved
@hal3e
Copy link
Contributor Author

hal3e commented Apr 25, 2024

So configurables are broken until the sway issue is fixed? Do we plan on releasing this before the fix for whatever cyclic-dep reason?

IMO we should wait until we have support for configurables with the new encoding.

@digorithm
Copy link
Member

So configurables are broken until the sway issue is fixed? Do we plan on releasing this before the fix for whatever cyclic-dep reason?

IMO we should wait until we have support for configurables with the new encoding.

Can we not wait for it, instead? The support for the new encoding in configurables might take a few days or a week, and it might delay the testnet release. Can we push this without supporting configurables (i.e., configurables would still use only the legacy encoding) for now, and add it later?

@digorithm digorithm added the epic An epic is a high-level master issue for large pieces of work. label May 1, 2024
segfault-magnet
segfault-magnet previously approved these changes May 2, 2024
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.

Let's get this merged, it's blocking the core update.

@hal3e
Copy link
Contributor Author

hal3e commented May 2, 2024

So configurables are broken until the sway issue is fixed? Do we plan on releasing this before the fix for whatever cyclic-dep reason?

IMO we should wait until we have support for configurables with the new encoding.

Can we not wait for it, instead? The support for the new encoding in configurables might take a few days or a week, and it might delay the testnet release. Can we push this without supporting configurables (i.e., configurables would still use only the legacy encoding) for now, and add it later?

yes we can do that but not in this PR as here the legacy encoding is completely gone. Here is the new PR: #1357

@hal3e hal3e mentioned this pull request May 2, 2024
5 tasks
@hal3e hal3e marked this pull request as ready for review May 6, 2024 11:41
segfault-magnet
segfault-magnet previously approved these changes May 6, 2024
Salka1988
Salka1988 previously approved these changes May 6, 2024
…unction_generator.rs

Co-authored-by: MujkicA <32431923+MujkicA@users.noreply.github.com>
@hal3e hal3e dismissed stale reviews from MujkicA, segfault-magnet, and Salka1988 via b2d5638 May 7, 2024 10:49
@hal3e hal3e merged commit 8df71c3 into master May 7, 2024
41 checks passed
@hal3e hal3e deleted the hal3e/remove-legacy-encoding branch May 7, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
big blocked breaking Introduces or requires breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor the sdk after the new encoding stabilizes
5 participants