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.

Sign up for GitHub

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: Use correct account in ClaimableBalanceID() #3406

Merged
merged 2 commits into from
Feb 16, 2021
Merged

txnbuild: Use correct account in ClaimableBalanceID() #3406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f2b347
Use correct account in ClaimableBalanceID()
tamirms Feb 12, 2021
96897b5
Add test case
tamirms Feb 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions txnbuild/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
All notable changes to this project will be documented in this
file. This project adheres to [Semantic Versioning](http://semver.org/).

## [v5.0.1](https://github.com/stellar/go/releases/tag/horizonclient-v5.0.1) - 2021-02-12

* Fix a bug in `ClaimableBalanceID()` where the wrong account was used to derive the claimable balance id ([#3404](https://github.com/stellar/go/pull/3404))
Shaptic marked this conversation as resolved.
Show resolved Hide resolved

## [v5.0.0](https://github.com/stellar/go/releases/tag/horizonclient-v5.0.0) - 2020-11-12

### Breaking changes
Expand Down
47 changes: 47 additions & 0 deletions txnbuild/create_claimable_balance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ package txnbuild

import (
"testing"
"time"

"github.com/stellar/go/keypair"
"github.com/stellar/go/network"
"github.com/stretchr/testify/assert"
)

func TestCreateClaimableBalanceRoundTrip(t *testing.T) {
Expand Down Expand Up @@ -31,3 +36,45 @@ func TestCreateClaimableBalanceRoundTrip(t *testing.T) {

roundTrip(t, []Operation{createNativeBalance, createAssetBalance})
}

func TestClaimableBalanceID(t *testing.T) {
A := "SCZANGBA5YHTNYVVV4C3U252E2B6P6F5T3U6MM63WBSBZATAQI3EBTQ4"
B := "GA2C5RFPE6GCKMY3US5PAB6UZLKIGSPIUKSLRB6Q723BM2OARMDUYEJ5"

aKeys := keypair.MustParseFull(A)
aAccount := SimpleAccount{AccountID: aKeys.Address(), Sequence: 123}

soon := time.Now().Add(time.Second * 60)
bCanClaim := BeforeRelativeTimePredicate(60)
aCanReclaim := NotPredicate(BeforeAbsoluteTimePredicate(soon.Unix()))

claimants := []Claimant{
NewClaimant(B, &bCanClaim),
NewClaimant(aKeys.Address(), &aCanReclaim),
}

claimableBalanceEntry := CreateClaimableBalance{
Destinations: claimants,
Asset: NativeAsset{},
Amount: "420",
SourceAccount: &SimpleAccount{AccountID: "GB56OJGSA6VHEUFZDX6AL2YDVG2TS5JDZYQJHDYHBDH7PCD5NIQKLSDO"},
}

// Build and sign the transaction
tx, err := NewTransaction(
TransactionParams{
SourceAccount: &aAccount,
IncrementSequenceNum: true,
BaseFee: MinBaseFee,
Timebounds: NewInfiniteTimeout(),
Operations: []Operation{&claimableBalanceEntry},
},
)
assert.NoError(t, err)
tx, err = tx.Sign(network.TestNetworkPassphrase, aKeys)
assert.NoError(t, err)

balanceId, err := tx.ClaimableBalanceID(0)
assert.NoError(t, err)
assert.Equal(t, "0000000095001252ab3b4d16adbfa5364ce526dfcda03cb2258b827edbb2e0450087be51", balanceId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you determine this ID to compare against?

This depends on your answer to the above, but do you think it'd be possible to make/modify one of the integration tests such that it actually fails without your changes? The existing TestProtocol15Basics/BalanceIDs test passes without this change, which obviously led to a false sense of security in the feature's correctness, so it'd be good to have deeper validation for this fix in future test runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test will fail without my changes because the claimable balance operation source account is different from the transaction source account

}
12 changes: 3 additions & 9 deletions txnbuild/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,18 +353,12 @@ func (t *Transaction) ClaimableBalanceID(operationIndex int) (string, error) {
return "", errors.New("invalid operation index")
}

operation, ok := t.operations[operationIndex].(*CreateClaimableBalance)
_, ok := t.operations[operationIndex].(*CreateClaimableBalance)
if !ok {
return "", errors.New("operation is not CreateClaimableBalance")
}

// Use the operation's source account or the transaction's source if not.
var account Account = &t.sourceAccount
if operation.SourceAccount != nil {
account = operation.GetSourceAccount()
}

seq, err := account.GetSequenceNumber()
seq, err := t.sourceAccount.GetSequenceNumber()
Shaptic marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return "", errors.Wrap(err, "failed to retrieve account sequence number")
}
Expand All @@ -374,7 +368,7 @@ func (t *Transaction) ClaimableBalanceID(operationIndex int) (string, error) {
operationId := xdr.OperationId{
Type: xdr.EnvelopeTypeEnvelopeTypeOpId,
Id: &xdr.OperationIdId{
SourceAccount: xdr.MustMuxedAddress(account.GetAccountID()),
SourceAccount: xdr.MustMuxedAddress(t.sourceAccount.GetAccountID()),
SeqNum: xdr.SequenceNumber(seq),
OpNum: xdr.Uint32(operationIndex),
},
Expand Down