#576: Add support for AMMClawback

[agutierrez0]

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]

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

1. create trust lines
2. send payments
3. create amm
4. deposit in amm
5. 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]

This is a good callout -- there does appear to be a lot of duplicated code. I'd be up for experimenting with how to make this test more concise with less duplicated code. Couple of thoughts:

1. We try to define commonly-used functions in AbstractIT.java, so take a look there to see if anything fits your needs. Basically, if there's a function that other ITs could use, then it probably makes sense to define those in AbstractIT. However, we simultaneously need to be judicious about this because sometimes it's clearer to simply create a Payment transaction using the builder (as your test does now) than to simply define a helper function that does the same thing (i.e., this merely shifts inputs around).
2. Instead of things like createPayment, I would look for higher-order functions to abstract away. E.g., createAmm seems like a good candidate, depending on the differences in usage in test cases. If you want to speak face-to-face sometime and experiment here, I'd be up for doing that. I think our ITs could use some refactoring/love anyway, and this might be a good use-case to experiment a bit with.
3. Despite 1 and 2 right above, we probably want to experiment with refactoring this IT in a separate PR, just so we can get this feature merged. What's in here already is probably good enough for this PR.

[agutierrez0]

@sappenin i have addressed your requested changes (format change requested today + changes requested on Feb 25th), could you please take a look?

[sappenin]

@agutierrez0 This PR is looking good, and feels very close. I think once we get the formatting worked out, and updates from main merged in, there's not much left.

Also, thanks for your patience in waiting for reviews on this one!

[sappenin]

This is a good callout -- there does appear to be a lot of duplicated code. I'd be up for experimenting with how to make this test more concise with less duplicated code. Couple of thoughts:

1. We try to define commonly-used functions in AbstractIT.java, so take a look there to see if anything fits your needs. Basically, if there's a function that other ITs could use, then it probably makes sense to define those in AbstractIT. However, we simultaneously need to be judicious about this because sometimes it's clearer to simply create a Payment transaction using the builder (as your test does now) than to simply define a helper function that does the same thing (i.e., this merely shifts inputs around).
2. Instead of things like createPayment, I would look for higher-order functions to abstract away. E.g., createAmm seems like a good candidate, depending on the differences in usage in test cases. If you want to speak face-to-face sometime and experiment here, I'd be up for doing that. I think our ITs could use some refactoring/love anyway, and this might be a good use-case to experiment a bit with.
3. Despite 1 and 2 right above, we probably want to experiment with refactoring this IT in a separate PR, just so we can get this feature merged. What's in here already is probably good enough for this PR.

[agutierrez0]

@sappenin

This is a good callout -- there does appear to be a lot of duplicated code. I'd be up for experimenting with how to make this test more concise with less duplicated code. Couple of thoughts:

Yeah I think being able to abstract some of this code for AMMs into function could reduce code duplication which could be nice. yeah would be great to chat, i will try to find a time on your calendar

[agutierrez0]

@sappenin I have addressed your requested changes, could you PTAL? i finally properly configured my formatting settings and cleaned up files that previously had changes. the PR is a lot cleaner now. pls let me know if anything else needs to be addressed

[sappenin]

@agutierrez0 All looks good. Just one small typo to fix in the Javadoc, and then I think we can merge this one.

(If you agree with the change, I think you can commit it directly from Github).

[sappenin]

@agutierrez0 I decided to just merge my suggestion. This is all set to merge. Once the build passes, I'll do that. Thanks for your contribution!!