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.

Sign up for GitHub

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

Jump to bottom

Add Credentials and PermissionedDomain tx Support #1118

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

Add Credentials and PermissionedDomain tx Support #1118

wants to merge 24 commits into from

Conversation

achowdhry-ripple
Copy link
Collaborator

@achowdhry-ripple achowdhry-ripple commented Jan 14, 2025 •
edited
Loading

High Level Overview of Change

Adds CredentialCreate, CredentialAccept, CredentialDelete, PermissionedDomainDelete, and PermissionedDomainSet transaction pages to the explorer.

Context of Change

https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0070d-credentials

https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0080d-permissioned-domains

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

CredentialAccept Simple
Screenshot 2025-02-20 at 1 29 42 PM

CredentialCreate Simple
Screenshot 2025-02-20 at 1 31 04 PM

CredentialDelete Simple
Screenshot 2025-02-20 at 1 31 50 PM

CredentialAccept TableDetail
Screenshot 2025-02-20 at 1 52 37 PM

CredentialCreate TableDetail
Screenshot 2025-02-20 at 1 53 56 PM

CredentialDelete TableDetail
Screenshot 2025-02-20 at 1 58 52 PM

PermissionedDomainSet Simple
Screenshot 2025-02-20 at 5 10 59 PM

PermissionedDomainDelete Simple
Screenshot 2025-02-20 at 5 11 56 PM

PermissionedDomainSet TableDetail

PermissionedDomainDelete TableDetail

Test Plan

kennyzlei and others added 3 commits December 18, 2024 10:06
@kennyzlei
fix: correct typo for website in english translation (#1101)
2af4952 
## High Level Overview of Change

Fix typo for website in english translation

### Context of Change

Bug introduced in #1056

### Type of Change

- [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

### TypeScript/Hooks Update

- [ ] Updated files to React Hooks
- [ ] Updated files to TypeScript
@achowdhry-ripple
basic structure of new transactions
b0dc30d
@achowdhry-ripple
simple tables completed
4c03487
@achowdhry-ripple achowdhry-ripple changed the title Add Credentials Transactions Draft: Add Credentials Transactions Jan 14, 2025
@achowdhry-ripple achowdhry-ripple changed the title Draft: Add Credentials Transactions Draft: Add Credentials and PermissionedDomain Support Jan 14, 2025
@achowdhry-ripple achowdhry-ripple changed the title Draft: Add Credentials and PermissionedDomain Support Draft: Add Credentials Support Jan 15, 2025
@pdp2121
Copy link
Collaborator

pdp2121 commented Jan 27, 2025

I would suggest creating TableDetails for these transactions as well

@achowdhry-ripple achowdhry-ripple changed the title Draft: Add Credentials Support Draft: Add Credentials and PermissionedDomain tx Support Jan 29, 2025
@achowdhry-ripple achowdhry-ripple marked this pull request as ready for review January 29, 2025 18:40
@achowdhry-ripple
Copy link
Collaborator Author

achowdhry-ripple commented Jan 29, 2025 •
edited
Loading

Pipeline is broken because this references PermissionedDomain tx from xrpl.js, which hasn't been released yet. Most of everything else is ready to review, but have not been able to test in livenet dev yet (waiting on PermissionedDomain to be released in dev)

@achowdhry-ripple achowdhry-ripple changed the title Draft: Add Credentials and PermissionedDomain tx Support Add Credentials and PermissionedDomain tx Support Jan 29, 2025
mvadari
mvadari reviewed Jan 29, 2025
View reviewed changes
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.

Are there supposed to be empty test files?

@achowdhry-ripple
Copy link
Collaborator Author

Are there supposed to be empty test files?

nope -- good callout, added the test jsons

@achowdhry-ripple
Copy link
Collaborator Author

Will redo the PermissionedDomain tests once its on devnet in a couple of days, hard to format right now

mvadari
mvadari reviewed Jan 31, 2025
View reviewed changes
src/containers/shared/components/Transaction/PermissionedDomainSet/index.ts
export const PermissionedDomainSetTransaction: TransactionMapping = {
Simple,
action: TransactionAction.MODIFY,
category: TransactionCategory.ACCOUNT,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This category doesn't feel right, but I'm not sure what is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pdp2121 any thoughts here? Not sure what the convention we'd follow for this is

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since DID falls into ACCOUNT, I think it's fine to have Credentials fall into the same category

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will leave this as is then

mvadari
mvadari reviewed Jan 31, 2025
View reviewed changes
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.

Still have one empty file

@mvadari
Copy link
Collaborator

mvadari commented Jan 31, 2025

Please add screenshots to the PR description

pdp2121
pdp2121 reviewed Feb 21, 2025
View reviewed changes
src/containers/shared/components/Transaction/CredentialCreate/TableDetail.tsx
{expiration && (
<div data-test="expiration">
<span className="label">{t('expiration')}: </span>
<span className="case-sensitive">{expiration}</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Expiration would be in Ripple epoch time. I think conversion and formatting are needed here

khancode
khancode approved these changes Feb 21, 2025
View reviewed changes
Copy link
Collaborator

@khancode khancode left a comment

Choose a reason for hiding this comment

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

LGTM as far as I can tell 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvadari mvadari mvadari left review comments

@pdp2121 pdp2121 pdp2121 left review comments

@khancode khancode khancode approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@achowdhry-ripple @pdp2121 @mvadari @khancode @kennyzlei