-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
chore: seal traits #1276
Conversation
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this 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. 🤔
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