Refactor xdr.LedgerEntry.LedgerKey method (#4942)

* Simplify ScError.Equals

* Add missing case for ScVal.Equals

* Add LedgerEntry.SetContractData and LedgerEntry.SetContractCode methods

* update generated xdr to 0f5e556

* Add LedgerCloseMetaV2 support

* Update ledgerTransaction.GetOperationEvents

* Make LedgerChangeReader emit evictions

* Fixing up after merge

* Add test for LedgerChangeReader extensions and evictions

* Fix typo

* Add xdr.LedgerEntryData.ExpirationLedgerSeq helper

* review feedback

* Refactor LedgerEntry.LedgerKey method to avoid panics, and only have a single copy

* Fix govet.sh

* Fixing govet

* Fix bug

* Remove unneeded code bits

* s/marshalling/marshaling

* Review feedback

gxdr/dump.go

@@ -16,7 +16,7 @@ func Dump(v goxdr.XdrType) []byte {
 }
 
 // Convert serializes the given goxdr value into another destination value
-// which supports binary unmarshalling.
+// which supports binary unmarshaling.
 //
 // This function can be used to convert github.com/xdrpp/goxdr/xdr values into
 // equivalent https://github.com/stellar/go-xdr values.

ingest/change_compactor.go

@@ -84,9 +84,13 @@ func (c *ChangeCompactor) AddChange(change Change) error {
 // change is unexpected.
 func (c *ChangeCompactor) addCreatedChange(change Change) error {
 	// safe, since we later cast to string (causing a copy)
-	ledgerKey, err := c.encodingBuffer.UnsafeMarshalBinary(change.Post.LedgerKey())
+	key, err := change.Post.LedgerKey()
 	if err != nil {
-		return errors.Wrap(err, "Error MarshalBinary")
+		return errors.Wrap(err, "error getting ledger key for new entry")
+	}
+	ledgerKey, err := c.encodingBuffer.UnsafeMarshalBinary(key)
+	if err != nil {
+		return errors.Wrap(err, "error marshaling ledger key for new entry")
 	}
 
 	ledgerKeyString := string(ledgerKey)
@@ -112,7 +116,7 @@ func (c *ChangeCompactor) addCreatedChange(change Change) error {
 		// If existing type is removed it means that this entry does exist
 		// in a DB so we update entry change.
 		c.cache[ledgerKeyString] = Change{
-			Type: change.Post.LedgerKey().Type,
+			Type: key.Type,
 			Pre:  existingChange.Pre,
 			Post: change.Post,
 		}
@@ -127,9 +131,13 @@ func (c *ChangeCompactor) addCreatedChange(change Change) error {
 // change is unexpected.
 func (c *ChangeCompactor) addUpdatedChange(change Change) error {
 	// safe, since we later cast to string (causing a copy)
-	ledgerKey, err := c.encodingBuffer.UnsafeMarshalBinary(change.Post.LedgerKey())
+	key, err := change.Post.LedgerKey()
+	if err != nil {
+		return errors.Wrap(err, "error getting ledger key for updated entry")
+	}
+	ledgerKey, err := c.encodingBuffer.UnsafeMarshalBinary(key)
 	if err != nil {
-		return errors.Wrap(err, "Error MarshalBinary")
+		return errors.Wrap(err, "error marshaling ledger key for updated entry")
 	}
 
 	ledgerKeyString := string(ledgerKey)
@@ -145,13 +153,13 @@ func (c *ChangeCompactor) addUpdatedChange(change Change) error {
 		// If existing type is created it means that this entry does not
 		// exist in a DB so we update entry change.
 		c.cache[ledgerKeyString] = Change{
-			Type: change.Post.LedgerKey().Type,
+			Type: key.Type,
 			Pre:  existingChange.Pre, // = nil
 			Post: change.Post,
 		}
 	case xdr.LedgerEntryChangeTypeLedgerEntryUpdated:
 		c.cache[ledgerKeyString] = Change{
-			Type: change.Post.LedgerKey().Type,
+			Type: key.Type,
 			Pre:  existingChange.Pre,
 			Post: change.Post,
 		}
@@ -171,9 +179,13 @@ func (c *ChangeCompactor) addUpdatedChange(change Change) error {
 // change is unexpected.
 func (c *ChangeCompactor) addRemovedChange(change Change) error {
 	// safe, since we later cast to string (causing a copy)
-	ledgerKey, err := c.encodingBuffer.UnsafeMarshalBinary(change.Pre.LedgerKey())
+	key, err := change.Pre.LedgerKey()
+	if err != nil {
+		return errors.Wrap(err, "error getting ledger key for removed entry")
+	}
+	ledgerKey, err := c.encodingBuffer.UnsafeMarshalBinary(key)
 	if err != nil {
-		return errors.Wrap(err, "Error MarshalBinary")
+		return errors.Wrap(err, "error marshaling ledger key for removed entry")
 	}
 
 	ledgerKeyString := string(ledgerKey)
@@ -191,7 +203,7 @@ func (c *ChangeCompactor) addRemovedChange(change Change) error {
 		delete(c.cache, ledgerKeyString)
 	case xdr.LedgerEntryChangeTypeLedgerEntryUpdated:
 		c.cache[ledgerKeyString] = Change{
-			Type: change.Pre.LedgerKey().Type,
+			Type: key.Type,
 			Pre:  existingChange.Pre,
 			Post: nil,
 		}

ingest/checkpoint_change_reader.go

@@ -357,11 +357,18 @@ LoopBucketEntry:
 
 				// Generate a key
 				var key xdr.LedgerKey
+				var err error
 
 				switch entry.Type {
 				case xdr.BucketEntryTypeLiveentry, xdr.BucketEntryTypeInitentry:
 					liveEntry := entry.MustLiveEntry()
-					key = liveEntry.LedgerKey()
+					key, err = liveEntry.LedgerKey()
+					if err != nil {
+						r.readChan <- r.error(
+							errors.Wrapf(err, "Error generating ledger key for XDR record %d of hash '%s'", n, hash.String()),
+						)
+						return false
+					}
 				case xdr.BucketEntryTypeDeadentry:
 					key = entry.MustDeadEntry()
 				default:
@@ -396,6 +403,7 @@ LoopBucketEntry:
 		n++
 
 		var key xdr.LedgerKey
+		var err error
 
 		switch entry.Type {
 		case xdr.BucketEntryTypeMetaentry:
@@ -414,7 +422,13 @@ LoopBucketEntry:
 			continue LoopBucketEntry
 		case xdr.BucketEntryTypeLiveentry, xdr.BucketEntryTypeInitentry:
 			liveEntry := entry.MustLiveEntry()
-			key = liveEntry.LedgerKey()
+			key, err = liveEntry.LedgerKey()
+			if err != nil {
+				r.readChan <- r.error(
+					errors.Wrapf(err, "Error generating ledger key for XDR record %d of hash '%s'", n, hash.String()),
+				)
+				return false
+			}
 		case xdr.BucketEntryTypeDeadentry:
 			key = entry.MustDeadEntry()
 		default:
@@ -523,8 +537,12 @@ func (r *CheckpointChangeReader) Read() (Change, error) {
 	if result.e != nil {
 		return Change{}, errors.Wrap(result.e, "Error while reading from buckets")
 	}
+	entryType, err := result.entryChange.EntryType()
+	if err != nil {
+		return Change{}, errors.Wrap(err, "Error getting entry type")
+	}
 	return Change{
-		Type: result.entryChange.EntryType(),
+		Type: entryType,
 		Post: result.entryChange.State,
 	}, nil
 }

ingest/ledger_change_reader.go

@@ -126,7 +126,11 @@ func (r *LedgerChangeReader) Read() (Change, error) {
 		return r.Read()
 	case evictionChangesState:
 		// Get contract ledgerEntry evictions
-		changes, err := GetChangesFromLedgerEntryEvictions(r.ledgerCloseMeta.EvictedLedgerKeys())
+		keys, err := r.ledgerCloseMeta.EvictedLedgerKeys()
+		if err != nil {
+			return Change{}, err
+		}
+		changes, err := GetChangesFromLedgerEntryEvictions(keys)
 		if err != nil {
 			return Change{}, err
 		}

ingest/ledgerbackend/buffered_meta_pipe_reader.go

@@ -49,7 +49,7 @@ type metaResult struct {
 // It solves the following issues:
 //
 //   - Decouples buffering from stellarCoreRunner so it can focus on running core.
-//   - Decouples unmarshalling and buffering of LedgerCloseMeta's from CaptiveCore.
+//   - Decouples unmarshaling and buffering of LedgerCloseMeta's from CaptiveCore.
 //   - By adding buffering it allows unmarshaling the ledgers available in Stellar-Core
 //     while previous ledger are being processed.
 //   - Limits memory usage in case of large ledgers are closed by the network.
@@ -99,7 +99,7 @@ func (b *bufferedLedgerMetaReader) readLedgerMetaFromPipe() (*xdr.LedgerCloseMet
 	var xlcm xdr.LedgerCloseMeta
 	_, err = xlcm.DecodeFrom(b.decoder)
 	if err != nil {
-		return nil, errors.Wrap(err, "unmarshalling framed LedgerCloseMeta")
+		return nil, errors.Wrap(err, "unmarshaling framed LedgerCloseMeta")
 	}
 	return &xlcm, nil
 }

ingest/ledgerbackend/captive_core_backend_test.go

@@ -1020,7 +1020,7 @@ func TestCaptiveGetLedger_ErrReadingMetaResult(t *testing.T) {
 		}
 	}
 	metaChan <- metaResult{
-		err: fmt.Errorf("unmarshalling error"),
+		err: fmt.Errorf("unmarshaling error"),
 	}
 
 	ctx := context.Background()
@@ -1061,7 +1061,7 @@ func TestCaptiveGetLedger_ErrReadingMetaResult(t *testing.T) {
 
 	// try reading from an empty buffer
 	_, err = captiveBackend.GetLedger(ctx, 66)
-	tt.EqualError(err, "unmarshalling error")
+	tt.EqualError(err, "unmarshaling error")
 
 	// not closed even if there is an error getting ledger
 	tt.False(captiveBackend.closed)

ingest/verify/main.go

@@ -87,10 +87,13 @@ func (v *StateVerifier) GetLedgerEntries(count int) ([]xdr.LedgerEntry, error) {
 			}
 		}
 
-		ledgerKey := entry.LedgerKey()
+		ledgerKey, err := entry.LedgerKey()
+		if err != nil {
+			return entries, errors.Wrap(err, "Error marshaling ledgerKey")
+		}
 		key, err := v.encodingBuffer.MarshalBinary(ledgerKey)
 		if err != nil {
-			return entries, errors.Wrap(err, "Error marshalling ledgerKey")
+			return entries, errors.Wrap(err, "Error marshaling ledgerKey")
 		}
 
 		entry.Normalize()
@@ -117,17 +120,21 @@ func (v *StateVerifier) Write(entry xdr.LedgerEntry) error {
 	}
 
 	// safe, since we convert to string right away (causing a copy)
-	key, err := v.encodingBuffer.UnsafeMarshalBinary(actualEntry.LedgerKey())
+	key, err := actualEntry.LedgerKey()
+	if err != nil {
+		return errors.Wrap(err, "Error marshaling ledgerKey")
+	}
+	keyBinary, err := v.encodingBuffer.UnsafeMarshalBinary(key)
 	if err != nil {
-		return errors.Wrap(err, "Error marshalling ledgerKey")
+		return errors.Wrap(err, "Error marshaling ledgerKey")
 	}
-	keyString := string(key)
+	keyString := string(keyBinary)
 	expectedEntry, exist := v.currentEntries[keyString]
 	if !exist {
 		return ingest.NewStateError(errors.Errorf(
 			"Cannot find entry in currentEntries map: %s (key = %s)",
 			base64.StdEncoding.EncodeToString(actualEntryMarshaled),
-			base64.StdEncoding.EncodeToString(key),
+			base64.StdEncoding.EncodeToString(keyBinary),
 		))
 	}
 	delete(v.currentEntries, keyString)

ingest/verify/main_test.go

@@ -63,7 +63,8 @@ func (s *StateVerifierTestSuite) TestCurrentEntriesNotEmpty() {
 	entryBase64, err := xdr.MarshalBase64(entry)
 	s.Assert().NoError(err)
 
-	ledgerKey := entry.LedgerKey()
+	ledgerKey, err := entry.LedgerKey()
+	s.Assert().NoError(err)
 	ledgerKeyBase64, err := xdr.MarshalBase64(ledgerKey)
 	s.Assert().NoError(err)
 
@@ -125,7 +126,9 @@ func (s *StateVerifierTestSuite) TestTransformFunction() {
 	s.Assert().NoError(err)
 
 	// Check currentEntries
-	ledgerKey, err := accountEntry.LedgerKey().MarshalBinary()
+	key, err := accountEntry.LedgerKey()
+	s.Assert().NoError(err)
+	ledgerKey, err := key.MarshalBinary()
 	s.Assert().NoError(err)
 
 	// Account entry transformed and offer entry ignored
@@ -164,7 +167,8 @@ func (s *StateVerifierTestSuite) TestWriteEntryNotExist() {
 	entryBase64, err := xdr.MarshalBase64(entry)
 	s.Assert().NoError(err)
 
-	ledgerKey := entry.LedgerKey()
+	ledgerKey, err := entry.LedgerKey()
+	s.Assert().NoError(err)
 	ledgerKeyBase64, err := xdr.MarshalBase64(ledgerKey)
 	s.Assert().NoError(err)
 

protocols/federation/main.go

@@ -26,7 +26,7 @@ type Memo struct {
 }
 
 func (m Memo) MarshalJSON() ([]byte, error) {
-	// Memo after marshalling should always be a string
+	// Memo after marshaling should always be a string
 	value, err := json.Marshal(m.Value)
 	if err != nil {
 		return []byte{}, err

protocols/horizon/main_test.go

@@ -67,7 +67,7 @@ func TestTransactionJSONMarshal(t *testing.T) {
 // Test that a typical friendbot fund response can unmarshal to the Transaction
 // type. The horizonclient uses the Transaction type for friendbot responses
 // also, but their response is a slimmed down version of the full transaction
-// response. This test confirms there are no errors unmarshalling that slimmed
+// response. This test confirms there are no errors unmarshaling that slimmed
 // down version.
 func TestTransactionUnmarshalsFriendbotFund(t *testing.T) {
 	friendbotFundResponse := `{

services/horizon/internal/actions/transaction.go

@@ -172,7 +172,7 @@ func loadTransactionRecords(ctx context.Context, hq *history.Q, qp TransactionsQ
 			var resultXDR xdr.TransactionResult
 			err = xdr.SafeUnmarshalBase64(t.TxResult, &resultXDR)
 			if err != nil {
-				return nil, errors.Wrap(err, "unmarshalling tx result")
+				return nil, errors.Wrap(err, "unmarshaling tx result")
 			}
 
 			if !resultXDR.Successful() {

services/horizon/internal/db2/history/operation.go

@@ -49,7 +49,7 @@ func preprocessDetails(details string) ([]byte, error) {
 	for k, v := range dest {
 		if strings.HasSuffix(k, "_muxed_id") {
 			if vNumber, ok := v.(json.Number); ok {
-				// transform it into a string so that _muxed_id unmarshalling works with `,string` tags
+				// transform it into a string so that _muxed_id unmarshaling works with `,string` tags
 				// see https://github.com/stellar/go/pull/3716#issuecomment-867057436
 				dest[k] = vNumber.String()
 			}

services/horizon/internal/ingest/orderbook.go

@@ -233,11 +233,11 @@ func (o *OrderBookStream) verifyAllOffers(ctx context.Context, offers []xdr.Offe
 			offerRowXDR := offerToXDR(offerRow)
 			offerEntryBase64, err := o.encodingBuffer.MarshalBase64(&offerEntry)
 			if err != nil {
-				return false, errors.Wrap(err, "Error from marshalling offerEntry")
+				return false, errors.Wrap(err, "Error from marshaling offerEntry")
 			}
 			offerRowBase64, err := o.encodingBuffer.MarshalBase64(&offerRowXDR)
 			if err != nil {
-				return false, errors.Wrap(err, "Error from marshalling offerRowXDR")
+				return false, errors.Wrap(err, "Error from marshaling offerRowXDR")
 			}
 			if offerEntryBase64 != offerRowBase64 {
 				mismatch = true
@@ -290,11 +290,11 @@ func (o *OrderBookStream) verifyAllLiquidityPools(ctx context.Context, liquidity
 			}
 			liquidityPoolEntryBase64, err = o.encodingBuffer.MarshalBase64(&liquidityPoolEntry)
 			if err != nil {
-				return false, errors.Wrap(err, "Error from marshalling liquidityPoolEntry")
+				return false, errors.Wrap(err, "Error from marshaling liquidityPoolEntry")
 			}
 			liquidityPoolRowBase64, err = o.encodingBuffer.MarshalBase64(&liquidityPoolRowXDR)
 			if err != nil {
-				return false, errors.Wrap(err, "Error from marshalling liquidityPoolRowXDR")
+				return false, errors.Wrap(err, "Error from marshaling liquidityPoolRowXDR")
 			}
 			if liquidityPoolEntryBase64 != liquidityPoolRowBase64 {
 				mismatch = true

services/horizon/internal/ingest/processors/trades_processor.go

@@ -175,8 +175,14 @@ func (p *TradeProcessor) findOperationChange(tx ingest.LedgerTransaction, opidx
 	var change ingest.Change
 	for i := len(changes) - 1; i >= 0; i-- {
 		change = changes[i]
-		if change.Pre != nil && key.Equals(change.Pre.LedgerKey()) {
-			return change, nil
+		if change.Pre != nil {
+			preKey, err := change.Pre.LedgerKey()
+			if err != nil {
+				return ingest.Change{}, errors.Wrap(err, "could not determine ledger key for change")
+			}
+			if key.Equals(preKey) {
+				return change, nil
+			}
 		}
 	}
 	return ingest.Change{}, errors.Errorf("could not find operation for key %v", key)

services/horizon/internal/ingest/processors/trust_lines_processor.go

@@ -56,7 +56,7 @@ func trustLineLedgerKey(trustLineEntry xdr.TrustLineEntry) (string, error) {
 	}
 	ledgerKeyString, err = ledgerKey.MarshalBinaryBase64()
 	if err != nil {
-		return "", errors.Wrap(err, "Error marshalling ledger key")
+		return "", errors.Wrap(err, "Error marshaling ledger key")
 	}
 	return ledgerKeyString, nil
 }

services/horizon/internal/ingest/verify.go

@@ -205,13 +205,21 @@ func (s *system) verifyState(verifyAgainstLatestCheckpoint bool) error {
 				accounts = append(accounts, entry.Data.MustAccount().AccountId.Address())
 				totalByType["accounts"]++
 			case xdr.LedgerEntryTypeData:
-				data = append(data, *entry.LedgerKey().Data)
+				key, keyErr := entry.LedgerKey()
+				if keyErr != nil {
+					return errors.Wrap(keyErr, "entry.LedgerKey")
+				}
+				data = append(data, *key.Data)
 				totalByType["data"]++
 			case xdr.LedgerEntryTypeOffer:
 				offers = append(offers, int64(entry.Data.MustOffer().OfferId))
 				totalByType["offers"]++
 			case xdr.LedgerEntryTypeTrustline:
-				trustLines = append(trustLines, entry.LedgerKey().MustTrustLine())
+				key, keyErr := entry.LedgerKey()
+				if keyErr != nil {
+					return errors.Wrap(keyErr, "TrustlineEntry.LedgerKey")
+				}
+				trustLines = append(trustLines, key.MustTrustLine())
 				totalByType["trust_lines"]++
 			case xdr.LedgerEntryTypeClaimableBalance:
 				cBalances = append(cBalances, entry.Data.MustClaimableBalance().BalanceId)

services/horizon/internal/test/integration/integration.go

@@ -993,7 +993,7 @@ func (i *Test) LogFailedTx(txResponse proto.Transaction, horizonResult error) {
 
 	var txResult xdr.TransactionResult
 	err := xdr.SafeUnmarshalBase64(txResponse.ResultXdr, &txResult)
-	assert.NoErrorf(t, err, "Unmarshalling transaction failed.")
+	assert.NoErrorf(t, err, "Unmarshaling transaction failed.")
 	assert.Equalf(t, xdr.TransactionResultCodeTxSuccess, txResult.Result.Code,
 		"Transaction did not succeed: %d", txResult.Result.Code)
 }