Skip to content

fix: Add synthetic NFT fields to ledger RPC, refactor to have common helper function#5706

Open
Copilot wants to merge 2 commits intodevelopfrom
copilot/fix-5adea215-d850-4ab8-a595-b04e63e948a6
Open

fix: Add synthetic NFT fields to ledger RPC, refactor to have common helper function#5706
Copilot wants to merge 2 commits intodevelopfrom
copilot/fix-5adea215-d850-4ab8-a595-b04e63e948a6

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Aug 20, 2025

High Level Overview of Change

Refactor synthetic field injection by consolidating three commonly-called functions (insertDeliveredAmount, insertNFTSyntheticInJson, and insertMPTokenIssuanceID) into a single helper function insertAllSyntheticInJson. This improves code maintainability by reducing duplication and providing a single point of update for synthetic field processing.

This results in adding the synthetic nftoken_id and nftoken_offer_id to the ledger RPC.

Context of Change

Previously, the same three synthetic field injection functions were called together in multiple locations throughout the codebase:

  • src/xrpld/app/misc/NetworkOPs.cpp
  • src/xrpld/rpc/handlers/Tx.cpp
  • src/xrpld/rpc/handlers/AccountTx.cpp
  • src/xrpld/app/ledger/detail/LedgerToJson.cpp
  • src/xrpld/rpc/handlers/Simulate.cpp

This pattern created maintenance overhead, as any changes to synthetic field processing would need to be replicated across all locations. Additionally, LedgerToJson.cpp contained an outdated conditional check that only applied insertDeliveredAmount for PAYMENT and CHECK_CASH transactions. Removing this outdated check now also causes delivered_amount to be included for ttACCOUNT_DELETE transactions in the ledger RPC, making it consistent with the tx and account_tx RPC endpoints (which already used canHaveDeliveredAmount to correctly handle ttACCOUNT_DELETE).

The refactor consolidates this logic into a reusable helper function with two overloads to handle different parameter signatures (ReadView vs JsonContext).

API Impact

  • Public API: New feature (new methods and/or new fields)

The ledger RPC now returns nftoken_id, nftoken_offer_id, and delivered_amount (for ttACCOUNT_DELETE transactions) in transaction metadata, consistent with other RPC endpoints.

Before / After

Before: Each location manually called three functions:

insertDeliveredAmount(response[jss::meta], ledger, transaction, transactionMeta);
insertNFTSyntheticInJson(response, transaction, transactionMeta);
insertMPTokenIssuanceID(response[jss::meta], transaction, transactionMeta);

After: Single helper function call:

insertAllSyntheticInJson(response, ledger, transaction, transactionMeta);

Test Plan

The change preserves existing functionality by calling the same underlying functions in the same order. All synthetic field injection behavior remains identical. The consolidation was tested across all five modified files to ensure consistent usage of the new helper function.

This also involved adding tests to ensure that the NFT synthetic fields were added to the ledger and account_tx RPC.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@mvadari

This comment was marked as resolved.

This comment was marked as resolved.

Copilot AI changed the title [WIP] Fix this issue. The linked pages should help with figuring out how to add the relevant field in the relevant place. Add missing nftoken_id synthetic field to ledger RPC response Aug 20, 2025
@mvadari mvadari added the DraftRunCI Normally CI does not run on draft PRs. This opts in. label Aug 20, 2025
@mvadari

This comment was marked as resolved.

This comment was marked as resolved.

Copilot AI changed the title Add missing nftoken_id synthetic field to ledger RPC response Add missing nftoken_id synthetic field to ledger RPC response and enhance NFT testing Aug 20, 2025
@mvadari

This comment was marked as resolved.

This comment was marked as resolved.

@mvadari

This comment was marked as resolved.

This comment was marked as resolved.

Copilot AI changed the title Add missing nftoken_id synthetic field to ledger RPC response and enhance NFT testing Fix missing nftoken_id synthetic field in ledger RPC response by properly handling API version differences Aug 20, 2025
@mvadari

This comment was marked as resolved.

This comment was marked as resolved.

@mvadari

This comment was marked as resolved.

@mvadari

This comment was marked as resolved.

@tequdev

This comment was marked as resolved.

@mvadari

This comment was marked as resolved.

@mvadari mvadari changed the title fix: add synthetic NFT fields to ledger RPC, refactor to have common helper function fix: Add synthetic NFT fields to ledger RPC, refactor to have common helper function Mar 12, 2026
@mvadari mvadari force-pushed the copilot/fix-5adea215-d850-4ab8-a595-b04e63e948a6 branch from 5bc27f9 to 202e96d Compare March 16, 2026 17:11
@mvadari mvadari requested review from a1q123456 and godexsoft March 16, 2026 17:11
@github-actions

This comment was marked as resolved.

RPC::insertMPTokenIssuanceID(
txJson[jss::metaData], txn, {txn->getTransactionID(), fill.ledger.seq(), *stMeta});
// Insert all synthetic fields
RPC::insertAllSyntheticInJson(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's a change in behaviour. We'll insert delivered amount for ttACCOUNT_DELETE after the change, as per canHaveDeliveredAmount

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is that a problem? That seems like a fix to me

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is an intentional behavioral change. The old ttPAYMENT || ttCHECK_CASH conditional in LedgerToJson.cpp was outdated — canHaveDeliveredAmount (used internally by insertDeliveredAmount) already correctly handles ttACCOUNT_DELETE, and the other RPC endpoints (tx, account_tx) have always included delivered amount for ttACCOUNT_DELETE transactions. This change makes ledger RPC consistent with the rest.

RPC::insertMPTokenIssuanceID(
txJson[jss::meta], txn, {txn->getTransactionID(), fill.ledger.seq(), *stMeta});
// Insert all synthetic fields
RPC::insertAllSyntheticInJson(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as above — the ttACCOUNT_DELETE delivered amount behavior change is intentional, making ledger RPC consistent with tx and account_tx endpoints.

@mvadari
Copy link
Copy Markdown
Collaborator

mvadari commented Apr 2, 2026

@copilot please fix the merge conflicts and update the branch

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 2, 2026

@copilot please fix the merge conflicts and update the branch

Merged the develop branch into this branch to resolve the merge conflicts (commit 3559f06). The only actual conflict was in LedgerToJson.cpp where develop had re-added braces/formatting via clang-tidy while our branch had switched to insertAllSyntheticInJson. Kept our implementation while incorporating develop's formatting changes.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2026

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2026

⚠️ This PR contains unsigned commits. To get your PR merged, please sign them. ⚠️

If only the most recent commit is unsigned, you can run:

  1. Amend the commit: git commit --amend --no-edit -n -S
  2. Overwrite the commit: git push --force-with-lease

If multiple commits are unsigned, you can run:

  1. Go into interactive rebase mode: git rebase --interactive HEAD~<NUM_OF_COMMITS>,
    where NUM_OF_COMMITS is the number of most recent commits that will be available
    to edit.
  2. Change "pick" to "edit" for the commits you need to sign, and then save and exit.
  3. For each commit, run: git commit --amend --no-edit -n -S
  4. Continue the rebase: git rebase --continue
  5. Overwrite the commit(s): git push --force-with-lease

If you're new to commit signing, there are different ways to set it up:

Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

  1. Generate a GPG key
  2. Add the GPG key to your GitHub account
  3. Configure git to use your GPG key for commit signing
Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

  1. Generate an SSH key and add it to ssh-agent
  2. Add the SSH key to your GitHub account
  3. Configure git to use your SSH key for commit signing
Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.
See use 1Password to sign your commits.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 3, 2026

This PR has conflicts, please resolve them in order for the PR to be reviewed.

Fix NFT synthetic field insertion for different API versions

Fix NFT test failures by handling API version metadata field differences

[Claude] replace with a helper function

[Claude] fix tests

pre-commit fixes

refactor: replace individual synthetic field functions with insertAllSyntheticInJson helper in simulate RPC

Co-authored-by: mvadari <8029314+mvadari@users.noreply.github.com>
@mvadari mvadari force-pushed the copilot/fix-5adea215-d850-4ab8-a595-b04e63e948a6 branch from 3559f06 to 3706565 Compare April 3, 2026 14:22
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 3, 2026

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistant minted nftoken_id field in API response (Version: 1.12.0)

5 participants