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

chore: seal traits #1276

Merged
merged 5 commits into from
Feb 16, 2024
Merged

chore: seal traits #1276

merged 5 commits into from
Feb 16, 2024

Conversation

MujkicA
Copy link
Contributor

@MujkicA MujkicA commented Feb 12, 2024

Following the discussions around #1244 I figured we could still benefit from sealing some of our public traits.
The benefit is that adding new methods to these traits will not cause a breaking change:

Transaction
TransactionBuilder
BuildableTransaction
EstimablePredicates
GasValidation

I've wondered wether Account is a trait where downstream implementations would make sense. I came up with a potential example of a joined account here. The current way to implement it is a bit clunky, especially because signing would require some kind of internal state to keep track of what accounts actually contributed to the input set. But if this is a a valid use case, we could consider improving the interface.

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.

@MujkicA MujkicA added the tech-debt Improves code quality or safety label Feb 12, 2024
@MujkicA MujkicA self-assigned this Feb 12, 2024
@MujkicA MujkicA marked this pull request as ready for review February 12, 2024 15:21
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.

Account is the only one that I'm not sure about.

I'm fine with whatever you choose. If you seal it and in hindsight it turns out to be a mistake, we can always unseal it.

If you don't seal it and we make a breaking change -- we're still able to breaking changes so fine again.

The use case you cited is interesting. As long as sealing Account doesn't prevent a use case for the sdk I'm ok with sealing it. The user can always manually create a transaction and fund it however he wishes.

@hal3e hal3e changed the title chore: Seal traits chore: seal traits Feb 13, 2024
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.

lgtm

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.

Neat little design pattern; I love it.

Now, for Account... maybe we shouldn't. I really liked the idea of joint accounts and enabling the users to implement those. 🤔

@MujkicA MujkicA merged commit b6c5a8b into master Feb 16, 2024
41 checks passed
@MujkicA MujkicA deleted the feature/sealing-traits branch February 16, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt Improves code quality or safety
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants