feat: add AMMClawback support #1127
Conversation
| <span className="label">{t('claws_back')}</span> | ||
| <Amount value={amount} displayIssuer /> | ||
| and | ||
| <Amount value={amount2} displayIssuer /> |
There was a problem hiding this comment.
Even if the flag tfClawTwoAssets is enabled, only one IOU is sent back to the issuer. The other asset in the pair is sent back to the holder account.
The difference occurs in the behavior of AMMWithdraw transaction -- both the assets are proportionally withdrawn instead of a single-asset withdrawal.
Is it possible to indicate this in the wording of the explorer?
Asset1 is clawed_back and sent to the Issuer1 whereas Asset2 is transferred back to the Holder
There was a problem hiding this comment.
I agree. Will fix this
There was a problem hiding this comment.
Hi @ckeshava, based on the description on xrpl.org for the tfClawTwoAssets:
Claw back the specified amount of Asset, and a corresponding amount of Asset2 based on the AMM pool's asset proportion; both assets must be issued by the issuer in the Account field. If this flag isn't enabled, the issuer claws back the specified amount of Asset, while a corresponding proportion of Asset2 goes back to the Holder.
I think both asset would be clawed back to the issuer with the flag enabled, otherwise there's no need for the condition that the second asset has to be issued by the issuer as well. I think my initial wording is correct here
There was a problem hiding this comment.
The exact behavior depends on whether the second asset was issued by the Issuer. I don't remember the initial wording in your PR. We can use the language from the official documentation verbatim.
| describe('AMMClawback: Simple', () => { | ||
| it('renders without tfClawTwoAssets flag, only one asset should be clawed back', () => { | ||
| const wrapper = createWrapper(mockAMMClawbackNoFlag) | ||
| expectSimpleRowText( |
There was a problem hiding this comment.
I'm trying to understand the difference between this file andsrc/containers/shared/components/Transaction/AMMClawback/test/AMMClawbackTableDetail.test.tsx. Both of them are testing the three different possibilities for the AMMClawback transaction. How are they different?
| "expected_date": null | ||
| "expected_date": null, | ||
| "transaction_type_name_AMMClawback": null, | ||
| "action_from_and": null |
There was a problem hiding this comment.
Is it possible to use a more informative name for this variable? what does this indicate action_from_and ? Could it be translate_amm_clawback_txn ?
There was a problem hiding this comment.
It is generic on purpose so others can re-use this pattern for any other purposes (if you look at other translation variables you will see the same pattern)
| @@ -557,5 +557,7 @@ | |||
| "search_results_banner": null, | |||
| "enable_amendment_name": null, | |||
| "amendment_status": null, | |||
| "expected_date": null | |||
| "expected_date": null, | |||
| "transaction_type_name_AMMClawback": null, | |||
There was a problem hiding this comment.
This should go up where the other transaction types are.
There was a problem hiding this comment.
I put these at the bottom for other translators to find easily but sure I can put it back to the group
src/containers/shared/components/Transaction/AMMClawback/Simple.tsx
Outdated
Show resolved
Hide resolved
src/containers/shared/components/Transaction/AMMClawback/TableDetail.tsx
Outdated
Show resolved
Hide resolved
|
I have addressed the comments. Ready for re-reviewing |
pdp2121
force-pushed
the
amm-clawback-support
branch
from
f834017 to
512d873
Compare
|
Is the |
High Level Overview of Change
Add AMMClawback transaction.
Spec: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0073-amm-clawback
Type of Change
Before / After
Simple page (without tfClawTwoAssets Flag)
Simple page (with tfClawTwoAssets Flag)
TableDetail (without tfClawTwoAssets Flag)
TableDetail (with tfClawTwoAssets Flag)