#576: Add support for AMMClawback #601

agutierrez0 · 2025-02-19T21:46:35Z

closes #576

add AmmClawback to SignatureUtils, Transaction.java, TransactionType.java, and TransactionTypeTests.java
- add test in SignatureUtils for the new AmmClawback class
create AmmClawbackFlags with only tfCalwTwoAssets flag or unset flags
create AmmClawback under transactions
generate new definitions.json
create new AmmClawbackTest transaction test
updated rippled container version to 2.3.0 from 2.2.0-rc3
enable new AMMClawback amendment in rippled.cfg
added integration tests under AmmIt
- one test where we clawback with amount
- one test where we clawback two assets using tfClawbackTwoAssets
- one test where we clawback without amount and only clawback one asset

agutierrez0 · 2025-02-19T22:11:38Z

xrpl4j-core/src/main/java/org/xrpl/xrpl4j/model/flags/AmmClawbackFlags.java

+  public boolean tfClawTwoAssets() {
+    return this.isSet(CLAW_TWO_ASSETS);
+  }


This is unused, i added it to follow the practice I saw with other TransactionFlag classes I saw (e.g. AmmWithdrawFlags). Let me know if I should remove.

agutierrez0 · 2025-02-19T22:16:30Z

xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/environment/RippledContainer.java

@@ -84,7 +82,7 @@ public class RippledContainer {
   * No-args constructor.
   */
  public RippledContainer() {
-    try (GenericContainer<?> container = new GenericContainer<>("rippleci/rippled:2.2.0")) {
+    try (GenericContainer<?> container = new GenericContainer<>("rippleci/rippled:2.3.0")) {


Let me know if there is another version I could use here instead of 2.3.0

I think version 2.3.1 is the most recent version, so we should use that

agutierrez0 · 2025-02-19T22:24:36Z

xrpl4j-core/src/main/java/org/xrpl/xrpl4j/model/transactions/AmmClawback.java

+  @JsonProperty("Flags")
+  @Value.Default
+  default AmmClawbackFlags flags() {
+    return AmmClawbackFlags.UNSET;
+  }


I noticed there was only one flag AMMClawback can be set to, this tfClawTwoAssets so I set the default to be UNSET, but if I should instead just make this an Optional<> field, let me know.

To align with other Transaction sub-types, this should default to AmmClawbackFlags.empty()

agutierrez0 · 2025-02-19T22:28:55Z

xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/AmmIT.java

For the three integration tests that I wrote, i would say around 90% of the code is the exact same:

create trust lines

send payments

create amm

deposit in amm

clawback assets in AMM

the only difference really is in the flags and/or the inclusion of amount in the final AMMClawback tx.

should I break down that that 90% into something more reusable?

e.g.

create the following functions:

createTrustline

createPayment

depositToAmm

etc..

adjust the createAmm function so that it can be reused

i had started going down this path (like what I did when changing enableRippling to enableFlag), but wanted to get an opinion first.

sappenin · 2025-02-26T00:35:35Z

xrpl4j-core/src/main/java/org/xrpl/xrpl4j/model/transactions/AmmClawback.java

+import org.xrpl.xrpl4j.model.flags.AmmClawbackFlags;
+import org.xrpl.xrpl4j.model.flags.TransactionFlags;
+import org.xrpl.xrpl4j.model.ledger.Issue;
+


Suggested change

import org.xrpl.xrpl4j.model.transactions.Address;

i have added this extra import, but on my IDE it shows as unused.

when i define the type for holder, i do see the Address references this path, but doesn't add the import. is there any reason why this happens?

xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/AmmIT.java

sappenin · 2025-02-26T00:43:12Z

xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/AmmIT.java

+    SubmitResult<TrustSet> trustlineSubmitResult2 = xrplClient.submit(signedTrustline2);
+    assertThat(trustlineSubmitResult2.engineResult()).isEqualTo(TransactionResultCodes.TES_SUCCESS);
+
+    // send payment of ZFT to trader wallet from issuer


Consider using real currency names, especially ones that are a familiar, like usd and xrp. It will be one less thing for developers to have keep track of while they seek to understand these tests.

i have replaced TST with USD and replaced ZFT with the existing token xrpl4j

and not sure why, i was able to build before committing... but after committing this last change, now my build constantly fails 😅

upd: build no longer failing

xrpl4j-core/src/test/java/org/xrpl/xrpl4j/model/transactions/AmmClawbackTest.java

sappenin · 2025-02-26T00:58:37Z

xrpl4j-core/src/main/resources/definitions.json

        "nth": 258,
-        "isVLEncoded": false,
-        "isSerialized": false,
-        "isSigningField": false,
        "type": "Amount"
      }


What version of rippled did you generate this definitions.json from?

i initially generated the definitions.json using this branch on the xrpl.js library. i compared it to the definitions.json that was commited to that repo after adding support for AMMClawback, and noticed mine was different so i used the same one the xrpl.js library used.

should i instead commit the file i get by running the script again on the latest in rippled's develop branch?

nkramer44 · 2025-02-27T14:47:35Z

xrpl4j-core/src/main/java/org/xrpl/xrpl4j/model/flags/AmmClawbackFlags.java

+/**
+ * {@link TransactionFlags} for {@link AmmClawback} transactions.
+ */
+public class AmmClawbackFlags extends TransactionFlags {


Looking at other TransactionFlags implementations, we always have a static empty() method which calls a private no-arg constructor. Check out OfferCreateFlags for reference

Also this class needs a test. Check out OfferCreateFlagsTests as an example

i have added this static empty() method and also added AmmClawbackFlagTest. thanks for the references 👍🏽 let me know what you think

nkramer44 · 2025-02-27T15:13:24Z

xrpl4j-core/src/main/java/org/xrpl/xrpl4j/model/transactions/AmmClawback.java

+  @JsonProperty("Flags")
+  @Value.Default
+  default AmmClawbackFlags flags() {
+    return AmmClawbackFlags.UNSET;
+  }


To align with other Transaction sub-types, this should default to AmmClawbackFlags.empty()

xrpl4j-core/src/test/java/org/xrpl/xrpl4j/crypto/signing/SignatureUtilsTest.java

nkramer44 · 2025-02-27T15:16:10Z

xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/AmmIT.java

@@ -60,10 +65,11 @@ public class AmmIT extends AbstractIT {

  static boolean shouldNotRun() {
    return System.getProperty("useTestnet") != null ||
-      System.getProperty("useClioTestnet") != null;
+        System.getProperty("useClioTestnet") != null;


I think your formatting settings are off, I'll send you the intellij formatting template

nkramer44 · 2025-02-27T15:17:34Z

xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/environment/RippledContainer.java

@@ -84,7 +82,7 @@ public class RippledContainer {
   * No-args constructor.
   */
  public RippledContainer() {
-    try (GenericContainer<?> container = new GenericContainer<>("rippleci/rippled:2.2.0")) {
+    try (GenericContainer<?> container = new GenericContainer<>("rippleci/rippled:2.3.0")) {


I think version 2.3.1 is the most recent version, so we should use that

Angel Gutierrez added 12 commits February 9, 2025 14:40

generate definitions.json from xrpl.js script

8c0896e

add amm clawback transaction and transaction flags

f7ede3e

add ammclawback to signature utils

6208633

set up tests, almost full flow working

ad2caee

cleanup test

ccee59b

remove extra print lines

e526d46

make enable rippling private again

c8aab1f

enable AMMClawback amendment, create two integration tests

b1f8c42

add one more integration test

3149b86

add comment for unset flag

ee8054c

add some comments to AmmClawback class

21b1e9d

merge main into this branch

fcd0407

agutierrez0 requested review from nkramer44, nhartner and sappenin as code owners February 19, 2025 21:46

agutierrez0 commented Feb 19, 2025

View reviewed changes

sappenin requested changes Feb 26, 2025

View reviewed changes

sappenin reviewed Feb 26, 2025

View reviewed changes

Angel Gutierrez added 5 commits February 25, 2025 23:36

fix build by addressing TransactionFlags reference

10b2b50

add Address import to AmmClawback class

850fa74

fix all checkstyle issues, successful build

b030ff5

address issues in unit test

16dc361

address changes for currencies used

ec0c5d2

nkramer44 requested changes Feb 27, 2025

View reviewed changes

Angel Gutierrez added 3 commits February 27, 2025 19:05

add new empty method and add tests for AmmClawbackFlags

60396cd

change default for AmmClawbackFlags to empty instead of Unset

1c92ba8

add multi signature transaction helper test for signature utils

0513be4

Angel Gutierrez added 3 commits February 27, 2025 19:14

update rippled version and fix checkstyle issues

7d8f95d

fix amm clawback test class failing test

6f41e95

remove failing data driven test types

d699102

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#576: Add support for AMMClawback #601

#576: Add support for AMMClawback #601

agutierrez0 commented Feb 19, 2025 •

edited

Loading

agutierrez0 Feb 19, 2025

agutierrez0 Feb 19, 2025

nkramer44 Feb 27, 2025

agutierrez0 Feb 19, 2025

nkramer44 Feb 27, 2025

agutierrez0 Feb 19, 2025 •

edited

Loading

sappenin Feb 26, 2025

agutierrez0 Feb 26, 2025

sappenin Feb 26, 2025

agutierrez0 Feb 26, 2025

agutierrez0 Feb 28, 2025

sappenin Feb 26, 2025

agutierrez0 Feb 26, 2025

nkramer44 Feb 27, 2025

nkramer44 Feb 27, 2025

agutierrez0 Feb 28, 2025

nkramer44 Feb 27, 2025

nkramer44 Feb 27, 2025

nkramer44 Feb 27, 2025

#576: Add support for AMMClawback #601

Are you sure you want to change the base?

#576: Add support for AMMClawback #601

Conversation

agutierrez0 commented Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agutierrez0 Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agutierrez0 commented Feb 19, 2025 •

edited

Loading

agutierrez0 Feb 19, 2025 •

edited

Loading