txnbuild: Remove TxEnvelope() functions from txnbuild #3377
Conversation
tamirms
force-pushed
the
remove-tx-envelope
branch
from
7e2b9c6 to
4fd2ccc
Compare
There was a problem hiding this comment.
I think reducing the number of functions that return the same thing, and therefore reducing the API, is good.
I think returning a transaction envelope that shouldn't be modified by the caller is also okay. As you say this API already does this in multiple places.
I think the errorless panic function is okay, because as far as I can tell the SDK doesn't allow the panic case to occur unless there was a logical bug in the SDK because parsing an XDR containing and unknown envelope type would error earlier, and the code won't allow constructing a transaction with other envelope types.
I left comments inline containin a question (❓) and suggestion (💡). Regardless this looks 👍🏻. I defer to @stellar/horizon-committers.
txnbuild/transaction.go
Outdated
| env.V1 = new(xdr.TransactionV1Envelope) | ||
| *env.V1 = *t.envelope.V1 | ||
| env.V1.Signatures = t.signatures | ||
| case xdr.EnvelopeTypeEnvelopeTypeTxV0: | ||
| env.V0 = new(xdr.TransactionV0Envelope) | ||
| *env.V0 = *t.envelope.V0 |
There was a problem hiding this comment.
❓ Why does this change include copying the V0 and V1 to itself? The signatures aren't copied, or the original envelope object. If the function says it returns an object that shouldn't be modified I don't think you need to do this copy.
txnbuild/transaction.go
Outdated
| env.V1 = new(xdr.TransactionV1Envelope) | ||
| *env.V1 = *t.envelope.V1 | ||
| env.V1.Signatures = t.signatures | ||
| case xdr.EnvelopeTypeEnvelopeTypeTxV0: | ||
| env.V0 = new(xdr.TransactionV0Envelope) | ||
| *env.V0 = *t.envelope.V0 |
There was a problem hiding this comment.
💡 We might want to consider storing signatures directly on the envelope at the time the signatures are added. In multiple places we set them lazily on the envelope which I don't think is particularly intuitive. If we set them upfront functions like ToXDR would simply be:
return t.envelope
| @@ -260,18 +246,36 @@ func (t *Transaction) HashHex(network string) (string, error) { | |||
| return hashHex(t.envelope, network) | |||
| } | |||
|
|
|||
| func (t *Transaction) clone(signatures []xdr.DecoratedSignature) *Transaction { | |||
There was a problem hiding this comment.
This may be OK but this function does not clone objects behind pointers. The removed cloneEnvelope did so, so it's worth checking if any assumptions were not broken by this change.
There was a problem hiding this comment.
Transaction never modifies the transaction envelope and when ToXDR() returns the transaction envelope we assume the caller will not modify the transaction envelope
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder 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
Remove
TxEnvelope()fromTransactionandFeeBumpTransaction. NowToXDR()is the only function which produces XDR transaction envelopes.Why
It simplifies the API to just have one function which produces an XDR transaction envelope. Although it would be nice to have immutable transaction envelopes, the inconvenience of checking the error result might outweigh the benefit. Given that the
Signatures()andOperations()functions assume the caller will not modify the returned slice, I think it is ok for us to be consistent and hold the same assumption inToXDR().Known limitations
[N/A]