From 4fd2cccedc067826a88ffd521ebf7b0ae5d9adce Mon Sep 17 00:00:00 2001 From: tamirms Date: Thu, 28 Jan 2021 13:38:59 +0100 Subject: [PATCH 1/2] Remove TxEnvelope() functions from txnbuild --- txnbuild/CHANGELOG.md | 6 +++++ txnbuild/fee_bump_test.go | 2 +- txnbuild/manage_data_test.go | 2 +- txnbuild/signers_test.go | 11 +++----- txnbuild/transaction.go | 49 +++++++++++++----------------------- txnbuild/transaction_test.go | 4 +-- 6 files changed, 31 insertions(+), 43 deletions(-) diff --git a/txnbuild/CHANGELOG.md b/txnbuild/CHANGELOG.md index 33dd9e9d75..6f35cf293b 100644 --- a/txnbuild/CHANGELOG.md +++ b/txnbuild/CHANGELOG.md @@ -3,6 +3,12 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). +## Unreleased + +### Breaking changes + +* Remove `TxEnvelope()` functions from `Transaction` and `FeeBumpTransaction` to simplify the API. `ToXDR()` should be used instead of `TxEnvelope()` ([#3377](https://github.com/stellar/go/pull/3377)) + ## [v5.0.0](https://github.com/stellar/go/releases/tag/horizonclient-v5.0.0) - 2020-11-12 ### Breaking changes diff --git a/txnbuild/fee_bump_test.go b/txnbuild/fee_bump_test.go index 05290fce84..f8001e866e 100644 --- a/txnbuild/fee_bump_test.go +++ b/txnbuild/fee_bump_test.go @@ -387,7 +387,7 @@ func TestFeeBumpRoundTrip(t *testing.T) { outerHash, err := feeBumpTx.HashHex(network.TestNetworkPassphrase) assert.NoError(t, err) - env, err := feeBumpTx.TxEnvelope() + env := feeBumpTx.ToXDR() assert.NoError(t, err) assert.Equal(t, xdr.EnvelopeTypeEnvelopeTypeTxFeeBump, env.Type) assert.Equal(t, xdr.MustAddress(kp1.Address()), env.FeeBumpAccount().ToAccountId()) diff --git a/txnbuild/manage_data_test.go b/txnbuild/manage_data_test.go index 79b7e529ca..a4303e0afe 100644 --- a/txnbuild/manage_data_test.go +++ b/txnbuild/manage_data_test.go @@ -93,7 +93,7 @@ func TestManageDataRoundTrip(t *testing.T) { ) assert.NoError(t, err) - envelope, err := tx.TxEnvelope() + envelope := tx.ToXDR() assert.NoError(t, err) assert.Len(t, envelope.Operations(), 1) assert.Equal(t, xdr.String64(manageData.Name), envelope.Operations()[0].Body.ManageDataOp.DataName) diff --git a/txnbuild/signers_test.go b/txnbuild/signers_test.go index f9d158ea66..273c18c692 100644 --- a/txnbuild/signers_test.go +++ b/txnbuild/signers_test.go @@ -440,7 +440,6 @@ type transactionCommon interface { Signatures() []xdr.DecoratedSignature Hash(networkStr string) ([32]byte, error) Base64() (string, error) - TxEnvelope() (xdr.TransactionEnvelope, error) ToXDR() xdr.TransactionEnvelope } @@ -459,16 +458,10 @@ func assertBase64(t *testing.T, tx transactionCommon) string { base64, err := tx.Base64() assert.NoError(t, err) - envCopy, err := tx.TxEnvelope() - assert.NoError(t, err) - envCopyBase64, err := xdr.MarshalBase64(envCopy) - assert.NoError(t, err) - envRef := tx.ToXDR() envRefBase64, err := xdr.MarshalBase64(envRef) assert.NoError(t, err) - assert.Equal(t, base64, envCopyBase64) assert.Equal(t, base64, envRefBase64) return base64 @@ -489,6 +482,7 @@ func TestSigningImmutability(t *testing.T) { root, err = root.Sign(network.TestNetworkPassphrase, kp0) assert.NoError(t, err) rootB64 := assertBase64(t, root) + rootXDR := root.ToXDR() left, err := root.Sign(network.TestNetworkPassphrase, kp1) assert.NoError(t, err) @@ -517,6 +511,9 @@ func TestSigningImmutability(t *testing.T) { verifySignatures(t, left, kp0, kp1) assert.Equal(t, expectedRightB64, rightB64) verifySignatures(t, right, kp0, kp2) + + rootXDRB64, err := xdr.MarshalBase64(rootXDR) + assert.Equal(t, expectedRootB64, rootXDRB64) } func TestFeeBumpSigningImmutability(t *testing.T) { diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index bd7e7cdcd9..20d9287ffb 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -182,19 +182,6 @@ func marshallBase64(e xdr.TransactionEnvelope, signatures []xdr.DecoratedSignatu return base64.StdEncoding.EncodeToString(binary), nil } -func cloneEnvelope(e xdr.TransactionEnvelope, signatures []xdr.DecoratedSignature) (xdr.TransactionEnvelope, error) { - var clone xdr.TransactionEnvelope - binary, err := marshallBinary(e, signatures) - if err != nil { - return clone, errors.Wrap(err, "could not marshall envelope") - } - - if err = xdr.SafeUnmarshal(binary, &clone); err != nil { - return clone, errors.Wrap(err, "could not unmarshall envelope") - } - return clone, nil -} - // Transaction represents a Stellar transaction. See // https://www.stellar.org/developers/guides/concepts/transactions.html // A Transaction may be wrapped by a FeeBumpTransaction in which case @@ -313,21 +300,19 @@ func (t *Transaction) AddSignatureBase64(network, publicKey, signature string) ( return newTx, nil } -// TxEnvelope returns the a xdr.TransactionEnvelope instance which is -// equivalent to this transaction. -func (t *Transaction) TxEnvelope() (xdr.TransactionEnvelope, error) { - return cloneEnvelope(t.envelope, t.signatures) -} - -// ToXDR is like TxEnvelope except that the transaction envelope returned by ToXDR -// should not be modified because any changes applied to the transaction envelope may +// ToXDR returns the a xdr.TransactionEnvelope which is equivalent to this transaction. +// The envelope should not be modified because any changes applied may // affect the internals of the Transaction instance. func (t *Transaction) ToXDR() xdr.TransactionEnvelope { env := t.envelope switch env.Type { case xdr.EnvelopeTypeEnvelopeTypeTx: + 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 env.V0.Signatures = t.signatures default: panic("invalid transaction type: " + env.Type.String()) @@ -499,19 +484,15 @@ func (t *FeeBumpTransaction) AddSignatureBase64(network, publicKey, signature st return newTx, nil } -// TxEnvelope returns the a xdr.TransactionEnvelope instance which is -// equivalent to this transaction. -func (t *FeeBumpTransaction) TxEnvelope() (xdr.TransactionEnvelope, error) { - return cloneEnvelope(t.envelope, t.signatures) -} - -// ToXDR is like TxEnvelope except that the transaction envelope returned by ToXDR -// should not be modified because any changes applied to the transaction envelope may +// ToXDR returns the a xdr.TransactionEnvelope which is equivalent to this transaction. +// The envelope should not be modified because any changes applied may // affect the internals of the FeeBumpTransaction instance. func (t *FeeBumpTransaction) ToXDR() xdr.TransactionEnvelope { env := t.envelope switch env.Type { case xdr.EnvelopeTypeEnvelopeTypeTxFeeBump: + env.FeeBump = new(xdr.FeeBumpTransactionEnvelope) + *env.FeeBump = *t.envelope.FeeBump env.FeeBump.Signatures = t.signatures default: panic("invalid transaction type: " + env.Type.String()) @@ -786,11 +767,15 @@ func NewFeeBumpTransaction(params FeeBumpTransactionParams) (*FeeBumpTransaction if inner == nil { return nil, errors.New("inner transaction is missing") } - innerEnv, err := inner.TxEnvelope() - if err != nil { - return nil, errors.Wrap(err, "inner transaction envelope not found") + switch inner.envelope.Type { + case xdr.EnvelopeTypeEnvelopeTypeTx, xdr.EnvelopeTypeEnvelopeTypeTxV0: + default: + return nil, errors.Errorf("%s transactions cannot be fee bumped", inner.envelope.Type) } + + innerEnv := inner.ToXDR() if innerEnv.Type == xdr.EnvelopeTypeEnvelopeTypeTxV0 { + var err error inner, err = convertToV1(inner) if err != nil { return nil, errors.Wrap(err, "could not upgrade transaction from v0 to v1") diff --git a/txnbuild/transaction_test.go b/txnbuild/transaction_test.go index 1ffa4b9fd0..0cf33179bf 100644 --- a/txnbuild/transaction_test.go +++ b/txnbuild/transaction_test.go @@ -1053,7 +1053,7 @@ func TestHashHex(t *testing.T) { expected = "1b3905ba8c3c0ecc68ae812f2d77f27c697195e8daf568740fc0f5662f65f759" assert.Equal(t, expected, hashHex, "hex encoded hash should match") - txEnv, err := tx.TxEnvelope() + txEnv := tx.ToXDR() assert.NoError(t, err) assert.NotNil(t, txEnv, "transaction xdr envelope should not be nil") sourceAccountFromEnv := txEnv.SourceAccount().ToAccountId() @@ -1983,7 +1983,7 @@ func TestReadChallengeTx_forbidsMuxedAccounts(t *testing.T) { time.Hour, ) - env, err := tx.TxEnvelope() + env := tx.ToXDR() assert.NoError(t, err) aid := xdr.MustAddress(kp0.Address()) muxedAccount := xdr.MuxedAccount{ From d0ca6e9eff8eb56ac87e13fde4ea1edf6559616b Mon Sep 17 00:00:00 2001 From: tamirms Date: Mon, 1 Feb 2021 14:05:27 +0100 Subject: [PATCH 2/2] Code review feedback --- txnbuild/helpers_test.go | 2 + txnbuild/transaction.go | 119 +++++++++++++++++---------------------- 2 files changed, 53 insertions(+), 68 deletions(-) diff --git a/txnbuild/helpers_test.go b/txnbuild/helpers_test.go index 4a99adbc9f..2705ee7945 100644 --- a/txnbuild/helpers_test.go +++ b/txnbuild/helpers_test.go @@ -103,6 +103,7 @@ func newSignedFeeBumpTransaction( } func convertToV0(tx *Transaction) { + signatures := tx.Signatures() tx.envelope.V0 = &xdr.TransactionV0Envelope{ Tx: xdr.TransactionV0{ SourceAccountEd25519: *tx.envelope.SourceAccount().Ed25519, @@ -112,6 +113,7 @@ func convertToV0(tx *Transaction) { Memo: tx.envelope.Memo(), Operations: tx.envelope.Operations(), }, + Signatures: signatures, } tx.envelope.V1 = nil tx.envelope.Type = xdr.EnvelopeTypeEnvelopeTypeTxV0 diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index 20d9287ffb..577f88ac47 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -195,7 +195,6 @@ type Transaction struct { operations []Operation memo Memo timebounds Timebounds - signatures []xdr.DecoratedSignature } // BaseFee returns the per operation fee for this transaction. @@ -232,7 +231,7 @@ func (t *Transaction) Operations() []Operation { // Signatures returns the list of signatures attached to this transaction. // The contents of the returned slice should not be modified. func (t *Transaction) Signatures() []xdr.DecoratedSignature { - return t.signatures + return t.envelope.Signatures() } // Hash returns the network specific hash of this transaction @@ -247,18 +246,36 @@ func (t *Transaction) HashHex(network string) (string, error) { return hashHex(t.envelope, network) } +func (t *Transaction) clone(signatures []xdr.DecoratedSignature) *Transaction { + newTx := new(Transaction) + *newTx = *t + newTx.envelope = t.envelope + + switch newTx.envelope.Type { + case xdr.EnvelopeTypeEnvelopeTypeTx: + newTx.envelope.V1 = new(xdr.TransactionV1Envelope) + *newTx.envelope.V1 = *t.envelope.V1 + newTx.envelope.V1.Signatures = signatures + case xdr.EnvelopeTypeEnvelopeTypeTxV0: + newTx.envelope.V0 = new(xdr.TransactionV0Envelope) + *newTx.envelope.V0 = *t.envelope.V0 + newTx.envelope.V0.Signatures = signatures + default: + panic("invalid transaction type: " + newTx.envelope.Type.String()) + } + + return newTx +} + // Sign returns a new Transaction instance which extends the current instance // with additional signatures derived from the given list of keypair instances. func (t *Transaction) Sign(network string, kps ...*keypair.Full) (*Transaction, error) { - extendedSignatures, err := concatSignatures(t.envelope, network, t.signatures, kps...) + extendedSignatures, err := concatSignatures(t.envelope, network, t.Signatures(), kps...) if err != nil { return nil, err } - newTx := new(Transaction) - *newTx = *t - newTx.signatures = extendedSignatures - return newTx, nil + return t.clone(extendedSignatures), nil } // SignWithKeyString returns a new Transaction instance which extends the current instance @@ -275,60 +292,40 @@ func (t *Transaction) SignWithKeyString(network string, keys ...string) (*Transa // with HashX signature type. // See description here: https://www.stellar.org/developers/guides/concepts/multi-sig.html#hashx. func (t *Transaction) SignHashX(preimage []byte) (*Transaction, error) { - extendedSignatures, err := concatHashX(t.signatures, preimage) + extendedSignatures, err := concatHashX(t.Signatures(), preimage) if err != nil { return nil, err } - newTx := new(Transaction) - *newTx = *t - newTx.signatures = extendedSignatures - return newTx, nil + return t.clone(extendedSignatures), nil } // AddSignatureBase64 returns a new Transaction instance which extends the current instance // with an additional signature derived from the given base64-encoded signature. func (t *Transaction) AddSignatureBase64(network, publicKey, signature string) (*Transaction, error) { - extendedSignatures, err := concatSignatureBase64(t.envelope, t.signatures, network, publicKey, signature) + extendedSignatures, err := concatSignatureBase64(t.envelope, t.Signatures(), network, publicKey, signature) if err != nil { return nil, err } - newTx := new(Transaction) - *newTx = *t - newTx.signatures = extendedSignatures - return newTx, nil + return t.clone(extendedSignatures), nil } // ToXDR returns the a xdr.TransactionEnvelope which is equivalent to this transaction. // The envelope should not be modified because any changes applied may // affect the internals of the Transaction instance. func (t *Transaction) ToXDR() xdr.TransactionEnvelope { - env := t.envelope - switch env.Type { - case xdr.EnvelopeTypeEnvelopeTypeTx: - 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 - env.V0.Signatures = t.signatures - default: - panic("invalid transaction type: " + env.Type.String()) - } - - return env + return t.envelope } // MarshalBinary returns the binary XDR representation of the transaction envelope. func (t *Transaction) MarshalBinary() ([]byte, error) { - return marshallBinary(t.envelope, t.signatures) + return marshallBinary(t.envelope, t.Signatures()) } // Base64 returns the base 64 XDR representation of the transaction envelope. func (t *Transaction) Base64() (string, error) { - return marshallBase64(t.envelope, t.signatures) + return marshallBase64(t.envelope, t.Signatures()) } // ClaimableBalanceID returns the claimable balance ID for the operation at the given index within the transaction. @@ -395,7 +392,6 @@ type FeeBumpTransaction struct { maxFee int64 feeAccount string inner *Transaction - signatures []xdr.DecoratedSignature } // BaseFee returns the per operation fee for this transaction. @@ -416,7 +412,7 @@ func (t *FeeBumpTransaction) FeeAccount() string { // Signatures returns the list of signatures attached to this transaction. // The contents of the returned slice should not be modified. func (t *FeeBumpTransaction) Signatures() []xdr.DecoratedSignature { - return t.signatures + return t.envelope.FeeBumpSignatures() } // Hash returns the network specific hash of this transaction @@ -431,18 +427,24 @@ func (t *FeeBumpTransaction) HashHex(network string) (string, error) { return hashHex(t.envelope, network) } +func (t *FeeBumpTransaction) clone(signatures []xdr.DecoratedSignature) *FeeBumpTransaction { + newTx := new(FeeBumpTransaction) + *newTx = *t + newTx.envelope.FeeBump = new(xdr.FeeBumpTransactionEnvelope) + *newTx.envelope.FeeBump = *t.envelope.FeeBump + newTx.envelope.FeeBump.Signatures = signatures + return newTx +} + // Sign returns a new FeeBumpTransaction instance which extends the current instance // with additional signatures derived from the given list of keypair instances. func (t *FeeBumpTransaction) Sign(network string, kps ...*keypair.Full) (*FeeBumpTransaction, error) { - extendedSignatures, err := concatSignatures(t.envelope, network, t.signatures, kps...) + extendedSignatures, err := concatSignatures(t.envelope, network, t.Signatures(), kps...) if err != nil { return nil, err } - newTx := new(FeeBumpTransaction) - *newTx = *t - newTx.signatures = extendedSignatures - return newTx, nil + return t.clone(extendedSignatures), nil } // SignWithKeyString returns a new FeeBumpTransaction instance which extends the current instance @@ -459,56 +461,40 @@ func (t *FeeBumpTransaction) SignWithKeyString(network string, keys ...string) ( // with HashX signature type. // See description here: https://www.stellar.org/developers/guides/concepts/multi-sig.html#hashx. func (t *FeeBumpTransaction) SignHashX(preimage []byte) (*FeeBumpTransaction, error) { - extendedSignatures, err := concatHashX(t.signatures, preimage) + extendedSignatures, err := concatHashX(t.Signatures(), preimage) if err != nil { return nil, err } - newTx := new(FeeBumpTransaction) - *newTx = *t - newTx.signatures = extendedSignatures - return newTx, nil + return t.clone(extendedSignatures), nil } // AddSignatureBase64 returns a new FeeBumpTransaction instance which extends the current instance // with an additional signature derived from the given base64-encoded signature. func (t *FeeBumpTransaction) AddSignatureBase64(network, publicKey, signature string) (*FeeBumpTransaction, error) { - extendedSignatures, err := concatSignatureBase64(t.envelope, t.signatures, network, publicKey, signature) + extendedSignatures, err := concatSignatureBase64(t.envelope, t.Signatures(), network, publicKey, signature) if err != nil { return nil, err } - newTx := new(FeeBumpTransaction) - *newTx = *t - newTx.signatures = extendedSignatures - return newTx, nil + return t.clone(extendedSignatures), nil } // ToXDR returns the a xdr.TransactionEnvelope which is equivalent to this transaction. // The envelope should not be modified because any changes applied may // affect the internals of the FeeBumpTransaction instance. func (t *FeeBumpTransaction) ToXDR() xdr.TransactionEnvelope { - env := t.envelope - switch env.Type { - case xdr.EnvelopeTypeEnvelopeTypeTxFeeBump: - env.FeeBump = new(xdr.FeeBumpTransactionEnvelope) - *env.FeeBump = *t.envelope.FeeBump - env.FeeBump.Signatures = t.signatures - default: - panic("invalid transaction type: " + env.Type.String()) - } - - return env + return t.envelope } // MarshalBinary returns the binary XDR representation of the transaction envelope. func (t *FeeBumpTransaction) MarshalBinary() ([]byte, error) { - return marshallBinary(t.envelope, t.signatures) + return marshallBinary(t.envelope, t.Signatures()) } // Base64 returns the base 64 XDR representation of the transaction envelope. func (t *FeeBumpTransaction) Base64() (string, error) { - return marshallBase64(t.envelope, t.signatures) + return marshallBase64(t.envelope, t.Signatures()) } // InnerTransaction returns the Transaction which is wrapped by @@ -575,7 +561,6 @@ func transactionFromParsedXDR(xdrEnv xdr.TransactionEnvelope) (*GenericTransacti maxFee: xdrEnv.FeeBumpFee(), inner: innerTx.simple, feeAccount: feeBumpAccount.Address(), - signatures: xdrEnv.FeeBumpSignatures(), } return newTx, nil } @@ -599,7 +584,6 @@ func transactionFromParsedXDR(xdrEnv xdr.TransactionEnvelope) (*GenericTransacti operations: nil, memo: nil, timebounds: Timebounds{}, - signatures: xdrEnv.Signatures(), } if timeBounds := xdrEnv.TimeBounds(); timeBounds != nil { @@ -661,7 +645,6 @@ func NewTransaction(params TransactionParams) (*Transaction, error) { operations: params.Operations, memo: params.Memo, timebounds: params.Timebounds, - signatures: nil, } accountID, err := xdr.AddressToAccountId(tx.sourceAccount.AccountID) @@ -757,7 +740,7 @@ func convertToV1(tx *Transaction) (*Transaction, error) { if err != nil { return tx, err } - tx.signatures = signatures + tx.envelope.V1.Signatures = signatures return tx, nil }