-
Notifications
You must be signed in to change notification settings - Fork 503
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
txnbuild: add AddSignatureDecorated to FeeBumpTransaction #4174
txnbuild: add AddSignatureDecorated to FeeBumpTransaction #4174
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.
LGTM! 🎉
func (t *FeeBumpTransaction) AddSignatureDecorated(signature ...xdr.DecoratedSignature) (*FeeBumpTransaction, error) { | ||
extendedSignatures, err := concatSignatureDecorated(t.envelope, t.Signatures(), signature) | ||
if err != nil { | ||
return nil, err |
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.
generally prefer wrapping errors so the context isn't lost
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.
I often do that too, but there is a series of add signature functions of which this is just one variation that was missing on this type and this is how they all work, with the errors being well defined within the concat functions.
kp0 := newKeypair0() | ||
kp1 := newKeypair1() | ||
|
||
tx, err := NewTransaction(TransactionParams{ |
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.
this new transaction creation could go into a helper function since it seems to be used as the standard transaction across multiple tests
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.
It could, but then the tests would become more coupled together. I've found it is easier to maintain tests in the long run if they are mostly independent of each other. Helper functions can be helpful, but they will also narrow the inputs to tests, or balloon into big configurable things of their own. Also this way they act as an example of the public API as much as possible.
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
Add
AddSignatureDecorated
toFeeBumpTransaction
.Why
The function exists on
Transaction
but not onFeeBumpTransaction
. I am writing some code that needs to be able to add decorated signatures to fee bump transactions in the same way we can add them to regular transactions.Known limitations
N/A