Fix the hashCode and equals methods in Transaction and `FeeBump…

…Transaction`. (#566)
lightsail-network · Jan 16, 2024 · cb8678d · cb8678d
1 parent f0bac07
commit cb8678d
Show file tree

Hide file tree

Showing 7 changed files with 174 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,16 +8,17 @@ As this project is pre 1.0, breaking changes may happen for minor version bumps.
 * Support resource leeway parameter when simulating Soroban transactions. ([#561](https://github.com/stellar/java-stellar-sdk/pull/561))
 * Support for the new, optional `diagnosticEventsXdr` field on the `SorobanServer.sendTransaction` method. ([#564](https://github.com/stellar/java-stellar-sdk/pull/564))
 * Remove deprecated classes and methods. ([#565](https://github.com/stellar/java-stellar-sdk/pull/565))
+* Fix the `hashCode` and `equals` methods in `Transaction` and `FeeBumpTransaction`. ([#566](https://github.com/stellar/java-stellar-sdk/pull/566))
 * Add `TransactionBuilder#TransactionBuilder(Transaction)` constructor. ([#567](https://github.com/stellar/java-stellar-sdk/pull/567))
 
 ### Breaking changes
+* Fix the `hashCode` and `equals` methods in `Transaction` and `FeeBumpTransaction`, now they will compare based on the `signatureBase()`. ([#566](https://github.com/stellar/java-stellar-sdk/pull/566))
 * The types of the following fields have changed. ([#560](https://github.com/stellar/java-stellar-sdk/pull/560))
   | field                                   | before  | now  |
   | --------------------------------------- | ------- | ---- |
   | GetEventsRequest.startLedger            | String  | Long |
   | GetEventsResponse.EventInfo.ledger      | Integer | Long |
   | GetLatestLedgerResponse.protocolVersion | Integer | Long |
-
 * The following classes and methods have been marked as deprecated in previous releases, and now they have been removed. ([#565](https://github.com/stellar/java-stellar-sdk/pull/565))
   * `AccountResponse.Signer#getAccountId()` has been removed, use `AccountResponse.Signer#getKey()` instead.
   * `OffersRequestBuilder#forAccount(String)` has been removed, use `OffersRequestBuilder#forSeller(String)` instead.

diff --git a/src/main/java/org/stellar/sdk/AbstractTransaction.java b/src/main/java/org/stellar/sdk/AbstractTransaction.java
@@ -5,7 +5,6 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
-import lombok.EqualsAndHashCode;
 import lombok.Getter;
 import lombok.NonNull;
 import org.stellar.sdk.xdr.DecoratedSignature;
@@ -15,8 +14,6 @@
 import org.stellar.sdk.xdr.TransactionSignaturePayload;
 
 /** Abstract class for transaction classes. */
-@EqualsAndHashCode(exclude = {"accountConverter"})
-// TODO: maybe we should not exclude accountConverter from equals and hashCode
 public abstract class AbstractTransaction {
   /** The network that the transaction is to be submitted to. */
   @NonNull @Getter protected final Network network;

diff --git a/src/main/java/org/stellar/sdk/FeeBumpTransaction.java b/src/main/java/org/stellar/sdk/FeeBumpTransaction.java
@@ -2,7 +2,6 @@
 
 import java.util.ArrayList;
 import java.util.Arrays;
-import lombok.EqualsAndHashCode;
 import lombok.Getter;
 import lombok.NonNull;
 import org.stellar.sdk.xdr.DecoratedSignature;
@@ -20,7 +19,6 @@
  *     Transactions</a>
  */
 @Getter
-@EqualsAndHashCode(callSuper = true)
 public class FeeBumpTransaction extends AbstractTransaction {
   /** The max fee willing to be paid for this transaction. */
   private final long fee;
@@ -202,4 +200,21 @@ public FeeBumpTransaction build() {
           this.accountConverter, this.feeAccount, this.baseFee, this.innerTransaction);
     }
   }
+
+  @Override
+  public boolean equals(Object object) {
+    if (this == object) {
+      return true;
+    }
+    if (object == null || getClass() != object.getClass()) {
+      return false;
+    }
+    FeeBumpTransaction that = (FeeBumpTransaction) object;
+    return Arrays.equals(signatureBase(), that.signatureBase());
+  }
+
+  @Override
+  public int hashCode() {
+    return Arrays.hashCode(signatureBase());
+  }
 }
diff --git a/src/main/java/org/stellar/sdk/Transaction.java b/src/main/java/org/stellar/sdk/Transaction.java
@@ -3,7 +3,6 @@
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.util.Arrays;
-import lombok.EqualsAndHashCode;
 import lombok.Getter;
 import lombok.NonNull;
 import org.stellar.sdk.xdr.AccountID;
@@ -29,7 +28,6 @@
  * Represents <a href="https://developers.stellar.org/docs/glossary/transactions/"
  * target="_blank">Transaction</a> in Stellar network.
  */
-@EqualsAndHashCode(callSuper = true)
 public class Transaction extends AbstractTransaction {
   /** fee paid for transaction in stroops (1 stroop = 0.0000001 XLM). */
   @Getter private final long fee;
@@ -313,6 +311,25 @@ public TransactionEnvelope toEnvelopeXdr() {
     return xdr;
   }
 
+  /**
+   * Maintain backwards compatibility references to Transaction.Builder
+   *
+   * @see org.stellar.sdk.TransactionBuilder
+   * @deprecated will be removed in upcoming releases. Use <code>TransactionBuilder</code> instead.
+   */
+  public static class Builder extends TransactionBuilder {
+    public Builder(
+        AccountConverter accountConverter,
+        TransactionBuilderAccount sourceAccount,
+        Network network) {
+      super(accountConverter, sourceAccount, network);
+    }
+
+    public Builder(TransactionBuilderAccount sourceAccount, Network network) {
+      super(sourceAccount, network);
+    }
+  }
+
   /**
    * Returns true if this transaction is a Soroban transaction.
    *
@@ -328,4 +345,21 @@ public boolean isSorobanTransaction() {
         || op instanceof ExtendFootprintTTLOperation
         || op instanceof RestoreFootprintOperation;
   }
+
+  @Override
+  public boolean equals(Object object) {
+    if (this == object) {
+      return true;
+    }
+    if (object == null || getClass() != object.getClass()) {
+      return false;
+    }
+    Transaction that = (Transaction) object;
+    return Arrays.equals(signatureBase(), that.signatureBase());
+  }
+
+  @Override
+  public int hashCode() {
+    return Arrays.hashCode(signatureBase());
+  }
 }
diff --git a/src/test/java/org/stellar/sdk/FeeBumpTransactionTest.java b/src/test/java/org/stellar/sdk/FeeBumpTransactionTest.java
@@ -10,13 +10,13 @@
 
 public class FeeBumpTransactionTest {
 
-  private Transaction createInnerTransaction(int baseFee) {
+  private Transaction createInnerTransaction(int baseFee, Network network) {
     KeyPair source =
         KeyPair.fromSecretSeed("SCH27VUZZ6UAKB67BDNF6FA42YMBMQCBKXWGMFD5TZ6S5ZZCZFLRXKHS");
 
     Account account = new Account(source.getAccountId(), 2908908335136768L);
     Transaction inner =
-        new TransactionBuilder(AccountConverter.enableMuxed(), account, Network.TESTNET)
+        new TransactionBuilder(AccountConverter.enableMuxed(), account, network)
             .addOperation(
                 new PaymentOperation.Builder(
                         "GA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVSGZ",
@@ -33,7 +33,15 @@ private Transaction createInnerTransaction(int baseFee) {
   }
 
   private Transaction createInnerTransaction() {
-    return createInnerTransaction(Transaction.MIN_BASE_FEE);
+    return createInnerTransaction(Transaction.MIN_BASE_FEE, Network.TESTNET);
+  }
+
+  private Transaction createInnerTransaction(int baseFee) {
+    return createInnerTransaction(baseFee, Network.TESTNET);
+  }
+
+  private Transaction createInnerTransaction(Network network) {
+    return createInnerTransaction(Transaction.MIN_BASE_FEE, network);
   }
 
   @Test
@@ -222,7 +230,6 @@ public void testFeeBumpUpgradesInnerToV1() throws IOException {
     assertEquals(0, feeBump.getSignatures().size());
 
     assertEquals(EnvelopeType.ENVELOPE_TYPE_TX_V0, innerV0.toEnvelopeXdr().getDiscriminant());
-    assertNotEquals(innerV0, feeBump.getInnerTransaction());
     innerV0.setEnvelopeType(EnvelopeType.ENVELOPE_TYPE_TX);
     assertEquals(innerV0, feeBump.getInnerTransaction());
 
@@ -245,4 +252,42 @@ public void testFeeBumpUpgradesInnerToV1() throws IOException {
     assertEquals(1, feeBump.getSignatures().size());
     signer.verify(feeBump.hash(), feeBump.getSignatures().get(0).getSignature().getSignature());
   }
+
+  @Test
+  public void testHashCodeAndEquals() {
+    Transaction inner = createInnerTransaction();
+
+    FeeBumpTransaction feeBump0 =
+        new FeeBumpTransaction.Builder(AccountConverter.enableMuxed(), inner)
+            .setBaseFee(Transaction.MIN_BASE_FEE * 2)
+            .setFeeAccount("GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3")
+            .build();
+
+    // they get different account converters
+    FeeBumpTransaction feeBump1 =
+        new FeeBumpTransaction.Builder(AccountConverter.disableMuxed(), inner)
+            .setBaseFee(Transaction.MIN_BASE_FEE * 2)
+            .setFeeAccount("GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3")
+            .build();
+    assertEquals(feeBump0.hashCode(), feeBump1.hashCode());
+    assertEquals(feeBump0, feeBump1);
+
+    // they get different base fee
+    FeeBumpTransaction feeBump2 =
+        new FeeBumpTransaction.Builder(AccountConverter.enableMuxed(), inner)
+            .setBaseFee(Transaction.MIN_BASE_FEE * 3)
+            .setFeeAccount("GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3")
+            .build();
+    System.out.println(feeBump2.toEnvelopeXdr());
+    assertNotEquals(feeBump0, feeBump2);
+
+    // they get different network
+    FeeBumpTransaction feeBump3 =
+        new FeeBumpTransaction.Builder(
+                AccountConverter.enableMuxed(), createInnerTransaction(Network.PUBLIC))
+            .setBaseFee(Transaction.MIN_BASE_FEE * 2)
+            .setFeeAccount("GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3")
+            .build();
+    assertNotEquals(feeBump0, feeBump3);
+  }
 }
diff --git a/src/test/java/org/stellar/sdk/Sep10ChallengeTest.java b/src/test/java/org/stellar/sdk/Sep10ChallengeTest.java
@@ -1286,7 +1286,7 @@ public void testReadChallengeTransactionInvalidWebAuthDomainOperationValueIsNull
     Operation[] operations =
         new Operation[] {domainNameOperation, webAuthDomainOperation, otherDomainOperation};
     Transaction transaction =
-        new TransactionBuilder(AccountConverter.disableMuxed(), sourceAccount, network)
+        new TransactionBuilder(AccountConverter.enableMuxed(), sourceAccount, network)
             .setBaseFee(100 * operations.length)
             .addOperations(Arrays.asList(operations))
             .addMemo(Memo.none())

diff --git a/src/test/java/org/stellar/sdk/TransactionTest.java b/src/test/java/org/stellar/sdk/TransactionTest.java
@@ -3,6 +3,7 @@
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -436,4 +437,72 @@ public void testIsSorobanTransactionBumpSequenceOperation() {
             Network.TESTNET);
     assertFalse(transaction.isSorobanTransaction());
   }
+
+  @Test
+  public void testHashCodeAndEquals() {
+    KeyPair source =
+        KeyPair.fromSecretSeed("SCH27VUZZ6UAKB67BDNF6FA42YMBMQCBKXWGMFD5TZ6S5ZZCZFLRXKHS");
+
+    Account account = new Account(source.getAccountId(), 2908908335136768L);
+    BumpSequenceOperation operation = new BumpSequenceOperation.Builder(0L).build();
+
+    Transaction transaction1 =
+        new Transaction(
+            AccountConverter.enableMuxed(),
+            account.getAccountId(),
+            Transaction.MIN_BASE_FEE,
+            account.getIncrementedSequenceNumber(),
+            new org.stellar.sdk.Operation[] {operation},
+            null,
+            new TransactionPreconditions(
+                null, null, BigInteger.ZERO, 0, new ArrayList<SignerKey>(), null),
+            null,
+            Network.TESTNET);
+
+    // they get different account converters
+    Transaction transaction2 =
+        new Transaction(
+            AccountConverter.disableMuxed(),
+            account.getAccountId(),
+            Transaction.MIN_BASE_FEE,
+            account.getIncrementedSequenceNumber(),
+            new org.stellar.sdk.Operation[] {operation},
+            null,
+            new TransactionPreconditions(
+                null, null, BigInteger.ZERO, 0, new ArrayList<SignerKey>(), null),
+            null,
+            Network.TESTNET);
+    assertEquals(transaction1.hashCode(), transaction2.hashCode());
+    assertEquals(transaction1, transaction2);
+
+    // they get different memo
+    Transaction transaction3 =
+        new Transaction(
+            AccountConverter.enableMuxed(),
+            account.getAccountId(),
+            Transaction.MIN_BASE_FEE,
+            account.getIncrementedSequenceNumber(),
+            new org.stellar.sdk.Operation[] {operation},
+            Memo.text("not equal tx"),
+            new TransactionPreconditions(
+                null, null, BigInteger.ZERO, 0, new ArrayList<SignerKey>(), null),
+            null,
+            Network.TESTNET);
+    assertNotEquals(transaction1, transaction3);
+
+    // they get different network
+    Transaction transaction4 =
+        new Transaction(
+            AccountConverter.enableMuxed(),
+            account.getAccountId(),
+            Transaction.MIN_BASE_FEE,
+            account.getIncrementedSequenceNumber(),
+            new org.stellar.sdk.Operation[] {operation},
+            null,
+            new TransactionPreconditions(
+                null, null, BigInteger.ZERO, 0, new ArrayList<SignerKey>(), null),
+            null,
+            Network.PUBLIC);
+    assertNotEquals(transaction1, transaction4);
+  }
 }