From 5301316abf8897a7ede0277677f84ad6fcf1e723 Mon Sep 17 00:00:00 2001 From: George Kudrayvtsev Date: Fri, 29 Apr 2022 12:03:30 -0700 Subject: [PATCH 1/4] 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. --- services/horizon/internal/resourceadapter/transaction.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 } From 226a78842996c5e7fe76e6d9f465ac3f6bf11c0d Mon Sep 17 00:00:00 2001 From: George Kudrayvtsev Date: Fri, 29 Apr 2022 15:33:13 -0700 Subject: [PATCH 2/4] Be a string again --- services/horizon/internal/db2/history/main.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 } From 05e2e57a035ae8c9e4bdac06ad6b9f121cdf403f Mon Sep 17 00:00:00 2001 From: George Kudrayvtsev Date: Fri, 29 Apr 2022 15:33:39 -0700 Subject: [PATCH 3/4] Add test case to validate field omissions --- .../resourceadapter/transaction_test.go | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) 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{} From 9d0eb3bce3c9d95422a015b38cc7c0065f5c8389 Mon Sep 17 00:00:00 2001 From: George Kudrayvtsev Date: Fri, 29 Apr 2022 15:33:30 -0700 Subject: [PATCH 4/4] Omit max_ledger when it's 0 --- protocols/horizon/main.go | 2 +- .../internal/db2/history/transaction_ledger_bounds.go | 5 ++++- 2 files changed, 5 insertions(+), 2 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/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), } }