Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clawback transaction #764

Merged
merged 22 commits into from
Jul 10, 2023
Merged

Clawback transaction #764

merged 22 commits into from
Jul 10, 2023

Conversation

shawnxie999
Copy link
Collaborator

@shawnxie999 shawnxie999 commented Jun 29, 2023

High Level Overview of Change

If the tx is a success, the actual amount that was clawed back is returned and shown in all components.
If the tx failed, the attempted amount is shown in all components.

Context of Change

Clawback transaction. See https://github.com/XRPLF/XRPL-Standards/pull/104/files?short_path=cb82c33#diff-cb82c337d579659136224b35dbbaac97d0ffaa3cfaa79981f1bebf4f4888feb5

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

TypeScript/Hooks Update

  • Updated files to React Hooks
  • Updated files to TypeScript

Before / After

image

Successful Simple
image

Failed Simple
image

Success table detail
image

Failed table detail
image

Description
image

Test Plan

@shawnxie999 shawnxie999 marked this pull request as ready for review June 29, 2023 19:20
@shawnxie999 shawnxie999 requested a review from mvadari June 29, 2023 19:28
@ckniffen
Copy link
Collaborator

ckniffen commented Jun 29, 2023

Can you add a <TableDetail> and a <Description> component for the type? They should be really straight forward.

I think you can just put amount and holder for <TableDetail> and the <Description> should be a nice sentence describing the transaction.

Copy link
Collaborator

@mvadari mvadari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the difference in the amount shown (total actually clawed back vs total attempted to be clawed back), should there be a difference in the label? e.g. Amount vs Attempted Amount or something

@ckniffen
Copy link
Collaborator

ckniffen commented Jul 3, 2023

Given the difference in the amount shown (total actually clawed back vs total attempted to be clawed back), should there be a difference in the label? e.g. Amount vs Attempted Amount or something

Or treat it like partial payments with a callout?

@shawnxie999
Copy link
Collaborator Author

Given the difference in the amount shown (total actually clawed back vs total attempted to be clawed back), should there be a difference in the label? e.g. Amount vs Attempted Amount or something

How much would it matter? The only thing that's relevant in a successful clawback transaction is how much token that was actually clawed back. The only time when the actual amount is different from the specified is if the holder owns less token than the specified amount, in which case, the issuer claws back the entirety of the holder's supply.

The actual amount clawed back will always less than or equal to the specified amount. There is not much information lost here. And if the person wants to know what the request looked like, they could always look at the raw JSON.

const createWrapper = createSimpleWrapperFactory(Simple)

describe('Clawback', () => {
it('handles Clawback simple view ', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test where the clawback amount is less than the Amount field?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example JSON I've used does claw back less than specified.

@ckniffen ckniffen merged commit a8b5944 into ripple:staging Jul 10, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants