From df257759c96cbffa65234473384fd5eed2d8a7c4 Mon Sep 17 00:00:00 2001 From: George Date: Fri, 29 Apr 2022 16:27:22 -0700 Subject: [PATCH] services/horizon: Improve transaction precondition `omitempty` behavior (#4360) * Omit max_ledger when it's 0 * Drops minSeqAge from response when it's 0: Because we use a string for the field (to work around languages that don't natively support uint64 values) in the JSON response, but the field is a non-pointer in the XDR, it will always be set and will be 0 by default. Since it's a duration, 0 is effectively "not set", and so it should be omitted from the resulting precondition. Caveat: if someone explicitly sets it to 0 when building their tx, this is equivalent to not setting it, so they may be confused about its omission, but this is working as intended. --- protocols/horizon/main.go | 2 +- services/horizon/internal/db2/history/main.go | 3 +- .../db2/history/transaction_ledger_bounds.go | 5 +- .../internal/resourceadapter/transaction.go | 2 +- .../resourceadapter/transaction_test.go | 64 +++++++++++++++++++ 5 files changed, 72 insertions(+), 4 deletions(-) diff --git a/protocols/horizon/main.go b/protocols/horizon/main.go index 2c151b9a7c..2b99a5e068 100644 --- a/protocols/horizon/main.go +++ b/protocols/horizon/main.go @@ -552,7 +552,7 @@ type TransactionPreconditionsTimebounds struct { type TransactionPreconditionsLedgerbounds struct { MinLedger uint32 `json:"min_ledger"` - MaxLedger uint32 `json:"max_ledger"` + MaxLedger uint32 `json:"max_ledger,omitempty"` } // FeeBumpTransaction contains information about a fee bump transaction diff --git a/services/horizon/internal/db2/history/main.go b/services/horizon/internal/db2/history/main.go index 14f0704650..2fdd76b3f4 100644 --- a/services/horizon/internal/db2/history/main.go +++ b/services/horizon/internal/db2/history/main.go @@ -720,7 +720,8 @@ func (t *Transaction) HasPreconditions() bool { return !t.TimeBounds.Null || !t.LedgerBounds.Null || t.MinAccountSequence.Valid || - t.MinAccountSequenceAge.Valid || + (t.MinAccountSequenceAge.Valid && + t.MinAccountSequenceAge.String != "0") || t.MinAccountSequenceLedgerGap.Valid || len(t.ExtraSigners) > 0 } diff --git a/services/horizon/internal/db2/history/transaction_ledger_bounds.go b/services/horizon/internal/db2/history/transaction_ledger_bounds.go index 2a8207c91d..0a217940cd 100644 --- a/services/horizon/internal/db2/history/transaction_ledger_bounds.go +++ b/services/horizon/internal/db2/history/transaction_ledger_bounds.go @@ -79,6 +79,9 @@ func formatLedgerBounds(ledgerBounds *xdr.LedgerBounds) LedgerBounds { return LedgerBounds{ MinLedger: null.IntFrom(int64(ledgerBounds.MinLedger)), - MaxLedger: null.IntFrom(int64(ledgerBounds.MaxLedger)), + // elide max_ledger if it's 0 since that means no upper bound + MaxLedger: null.NewInt( + int64(ledgerBounds.MaxLedger), + ledgerBounds.MaxLedger > 0), } } diff --git a/services/horizon/internal/resourceadapter/transaction.go b/services/horizon/internal/resourceadapter/transaction.go index 76922fec6a..a72f202d36 100644 --- a/services/horizon/internal/resourceadapter/transaction.go +++ b/services/horizon/internal/resourceadapter/transaction.go @@ -84,7 +84,7 @@ func PopulateTransaction( dest.Preconditions.MinAccountSequence = fmt.Sprint(row.MinAccountSequence.Int64) } - if row.MinAccountSequenceAge.Valid { + if row.MinAccountSequenceAge.Valid && row.MinAccountSequenceAge.String != "0" { dest.Preconditions.MinAccountSequenceAge = row.MinAccountSequenceAge.String } diff --git a/services/horizon/internal/resourceadapter/transaction_test.go b/services/horizon/internal/resourceadapter/transaction_test.go index 45b3c556fd..9168f2c3e5 100644 --- a/services/horizon/internal/resourceadapter/transaction_test.go +++ b/services/horizon/internal/resourceadapter/transaction_test.go @@ -2,6 +2,7 @@ package resourceadapter import ( "encoding/base64" + "encoding/json" "fmt" "testing" "time" @@ -293,6 +294,69 @@ func TestPopulateTransaction_PreconditionsV2(t *testing.T) { } } +// TestPopulateTransaction_PreconditionsV2_Omissions ensures that all fields are +// omitted from the final JSON when appropriate. +func TestPopulateTransaction_PreconditionsV2_Omissions(t *testing.T) { + ctx, _ := test.ContextWithLogBuffer() + tt := assert.New(t) + + jsonifyTx := func(tx history.TransactionWithoutLedger) ( + Transaction, map[string]interface{}, + ) { + var dest Transaction + generic := map[string]interface{}{} + + row := history.Transaction{TransactionWithoutLedger: tx} + tt.NoError(PopulateTransaction(ctx, row.TransactionHash, &dest, row)) + + bytes, err := dest.MarshalJSON() + tt.NoError(err) + tt.NoError(json.Unmarshal(bytes, &generic)) // round-trip + + return dest, generic + } + + for _, tx := range []history.TransactionWithoutLedger{ + { + // minimum precondition so that the field exists in general + MinAccountSequenceLedgerGap: null.IntFrom(0), + TimeBounds: history.TimeBounds{Null: true}, + LedgerBounds: history.LedgerBounds{Null: true}, + MinAccountSequence: null.IntFromPtr(nil), + MinAccountSequenceAge: null.StringFrom("0"), + ExtraSigners: pq.StringArray{}, + }, { + MinAccountSequenceLedgerGap: null.IntFrom(0), + TimeBounds: history.TimeBounds{Null: true}, + LedgerBounds: history.LedgerBounds{Null: true}, + MinAccountSequence: null.IntFromPtr(nil), + MinAccountSequenceAge: null.StringFromPtr(nil), + ExtraSigners: nil, + }, + } { + dest, js := jsonifyTx(tx) + if !tt.Contains(js, "preconditions") || + !tt.IsType(map[string]interface{}{}, js["preconditions"]) { + tt.FailNow("expected 'preconditions' object in transaction") + } + + precond := js["preconditions"].(map[string]interface{}) + helpStr := fmt.Sprintf("input precondition: %+v", dest.Preconditions) + tt.NotContainsf(precond, "timebounds", helpStr) + tt.NotContainsf(precond, "ledgerbounds", helpStr) + tt.NotContainsf(precond, "min_account_sequence", helpStr) + tt.NotContainsf(precond, "min_account_sequence_age", helpStr) + tt.NotContainsf(precond, "min_account_sequence_ledger_gap", helpStr) + tt.NotContainsf(precond, "extra_signers", helpStr) + + // If we drop the minimum precondition, the structure should cease to + // exist entirely. + tx.MinAccountSequenceLedgerGap = null.IntFromPtr(nil) + dest, js = jsonifyTx(tx) + tt.NotContains(js, "preconditions") + } +} + func TestFeeBumpTransaction(t *testing.T) { ctx, _ := test.ContextWithLogBuffer() dest := Transaction{}