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

XLS-33 Multi Purpose token #989

Open
wants to merge 35 commits into
base: staging
Choose a base branch
from
Open

Conversation

shawnxie999
Copy link
Collaborator

@shawnxie999 shawnxie999 commented May 30, 2024

High Level Overview of Change

  • add MPT transactions: MPTokenIssuance, MPTokenIssuanceDestroy, MPTokenAuthorize, MPTokenIssuanceSet
  • add MPT page
  • support search by MPTID
  • updates transactions: Payment, Clawback
  • modified Currency to support MPTID

Context of Change

Spec: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0033d-multi-purpose-tokens

MPT page
image

Ledger view
image

MPTokenIssuance
image

MPTokenAuthorize
image

MPTokenIssuanceSet
image

Payment (with scaling MPT amount)
image

Clawback
image

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
  • Translation Updates
  • Release

TypeScript/Hooks Update

  • Updated files to React Hooks
  • Updated files to TypeScript

Before / After

Test Plan

@shawnxie999 shawnxie999 changed the title MPT MPT May 31, 2024
@shawnxie999 shawnxie999 changed the title MPT XLS-33 Multi Purpose token May 31, 2024

const AccountAssetTabDisconnected = ({ account }: Props) => {
const { id: accountId = '', assetType = assetTypes[0] } =
useRouteParams(ACCOUNT_ROUTE)

const supportsMPT = ['mpt_sandbox', 'devnet'].includes(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would assume this is just for testing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently we have an alt server for testing. But assuming if the amendment is activated on mainnet, then this code would need to be removed

@@ -0,0 +1,15 @@
@import '../shared/css/variables';

.mpt-page {
Copy link
Collaborator

@pdp2121 pdp2121 Jul 22, 2024

Choose a reason for hiding this comment

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

Not necessarily needed in this PR but we should create a template for token pages so that there'd be not a lot of copy & paste needed (same with the Header stylings)

@pdp2121
Copy link
Collaborator

pdp2121 commented Jul 24, 2024

I'm not sure which way is better: the current way of showing scaled amount, or just showed the amount divided by 10^scale (so that on payment and clawback simple page we dont have to click on the token itself to find out about the scale). Other than that, looks good to me.

@pdp2121
Copy link
Collaborator

pdp2121 commented Jul 24, 2024

The failed tests can be fixed when merging latest changes from staging

@mvadari
Copy link
Collaborator

mvadari commented Jul 24, 2024

I'm not sure which way is better: the current way of showing scaled amount, or just showed the amount divided by 10^scale (so that on payment and clawback simple page we dont have to click on the token itself to find out about the scale). Other than that, looks good to me.

Personally, I'd rather see the scaled amount instead of having to do the math myself.

@shawnxie999
Copy link
Collaborator Author

I'm not sure which way is better: the current way of showing scaled amount, or just showed the amount divided by 10^scale (so that on payment and clawback simple page we dont have to click on the token itself to find out about the scale). Other than that, looks good to me.

Personally, I'd rather see the scaled amount instead of having to do the math myself.

@mvadari @pdp2121 One problem with showing the amount divided by 10^scale that is the information of the AssetScale is stored in the MPTokenIssuance object. This means that whenever MPT amount is display, we would need to make an extra call using the ledger_entry API to fetch the MPTokenIssuance object to get the scale.

@mvadari
Copy link
Collaborator

mvadari commented Aug 22, 2024

This means that whenever MPT amount is display, we would need to make an extra call using the ledger_entry API to fetch the MPTokenIssuance object to get the scale.

I think that's fine.

pdp2121
pdp2121 previously approved these changes Aug 26, 2024
@pdp2121 pdp2121 dismissed their stale review August 26, 2024 21:38

Needs consensus on MPT amount display

@pdp2121
Copy link
Collaborator

pdp2121 commented Aug 26, 2024

This means that whenever MPT amount is display, we would need to make an extra call using the ledger_entry API to fetch the MPTokenIssuance object to get the scale.

I think that's fine.

Yes. I would prefer seeing the scaled amount. We are also showing scaled amount in PriceOracles tx.

pdp2121 and others added 2 commits September 3, 2024 11:49
…pled` (ripple#1033)

## High Level Overview of Change

<!--
Please include a summary/list of the changes.
If too broad, please consider splitting into multiple PRs.
-->
Resolve ripple#358

### Type of Change

<!--
Please check relevant options, delete irrelevant ones.
-->

- [x] 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
- [ ] Translation Updates
- [ ] Release

## Before / After

<!--
If just refactoring / back-end changes, this can be just an in-English
description of the change at a technical level.
If a UI change, screenshots should be included.
-->

### Before

![Screenshot 2024-08-26 at 4 51
58 PM](https://github.com/user-attachments/assets/7ead2467-486a-45df-ae42-009545ff626c)


### After

![Screenshot 2024-08-26 at 5 20
23 PM](https://github.com/user-attachments/assets/46a21400-90d2-45bd-b948-32d4770defb4)
@shawnxie999 shawnxie999 changed the base branch from staging to upgrade-status-stablize September 3, 2024 15:52
@shawnxie999 shawnxie999 changed the base branch from upgrade-status-stablize to staging September 3, 2024 15:52
@shawnxie999
Copy link
Collaborator Author

@pdp2121 @mvadari

updated scaling in this commit

it now looks like this
image
image

Copy link
Collaborator

@pdp2121 pdp2121 left a comment

Choose a reason for hiding this comment

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

LGTM. Just left a comment and may be get the linting fixed

@shawnxie999
Copy link
Collaborator Author

LGTM. Just left a comment and may be get the linting fixed

Not quite sure why the lint fails, the ones that are failing are the ones that my local linter told me to fix.

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