Add augmented submit fields to sign-and-submit mode#6304
Add augmented submit fields to sign-and-submit mode#6304
Conversation
Co-authored-by: mvadari <8029314+mvadari@users.noreply.github.com>
Co-authored-by: mvadari <8029314+mvadari@users.noreply.github.com>
Co-authored-by: mvadari <8029314+mvadari@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR ensures that the submit RPC returns the same augmented result fields in sign-and-submit mode as it already does in binary blob mode, fixing an inconsistency introduced in v1.5.0-b6.
Changes:
- Extended
transactionFormatResultImplinTransactionSign.cppto attach the augmented submit fields (accepted,applied,broadcast,queued,kept, plus ledger state fields) based onTransaction::getSubmitResult()andgetCurrentLedgerState(), mirroring the existing blobsubmithandler. - Added
Submit_testRPC tests to verify that both sign-and-submit and binary blob submission paths return the augmented fields.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/xrpld/rpc/detail/TransactionSign.cpp | Adds population of the augmented submit and current-ledger-state fields to sign-and-submit responses, aligning them with the binary blob submit behavior. |
| src/test/rpc/Submit_test.cpp | Introduces RPC tests that assert presence of the augmented fields in both sign-and-submit and binary submit flows, guarding against regressions of this bug. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto const submitResult = tpTrans->getSubmitResult(); | ||
|
|
||
| jvResult[jss::accepted] = submitResult.any(); | ||
| jvResult[jss::applied] = submitResult.applied; | ||
| jvResult[jss::broadcast] = submitResult.broadcast; | ||
| jvResult[jss::queued] = submitResult.queued; | ||
| jvResult[jss::kept] = submitResult.kept; | ||
|
|
||
| if (auto currentLedgerState = tpTrans->getCurrentLedgerState()) | ||
| { | ||
| jvResult[jss::account_sequence_next] = safe_cast<Json::Value::UInt>(currentLedgerState->accountSeqNext); | ||
| jvResult[jss::account_sequence_available] = | ||
| safe_cast<Json::Value::UInt>(currentLedgerState->accountSeqAvail); | ||
| jvResult[jss::open_ledger_cost] = to_string(currentLedgerState->minFeeRequired); | ||
| jvResult[jss::validated_ledger_index] = | ||
| safe_cast<Json::Value::UInt>(currentLedgerState->validatedLedger); | ||
| } |
There was a problem hiding this comment.
The logic that populates the augmented submit fields (accepted/applied/broadcast/queued/kept and the current ledger state fields) is now duplicated here and in rpc/handlers/Submit.cpp (lines 130–145). Duplicating this block in two places increases the risk that the fields diverge in the future if one call site is updated but the other is not. Consider extracting a small helper that takes a Transaction::pointer (and maybe apiVersion, if needed) and mutates a Json::Value with these fields, so both the binary-blob submit path and the sign-and-submit path share the exact same implementation.
There was a problem hiding this comment.
Extracted populateAugmentedSubmitFields helper function to eliminate code duplication. Both Submit.cpp and TransactionSign.cpp now use the same implementation. See commit d18a9e2.
|
…GELOG Co-authored-by: mvadari <8029314+mvadari@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6304 +/- ##
=======================================
Coverage 79.9% 79.9%
=======================================
Files 840 840
Lines 65483 65484 +1
Branches 7255 7247 -8
=======================================
+ Hits 52311 52327 +16
+ Misses 13172 13157 -15
🚀 New features to boost your workflow:
|
…ted-submit-fields
…ented-submit-fields
High Level Overview of Change
The
submitRPC returns augmented fields (added in #3125) only for binary blob submissions, not for sign-and-submit mode. This PR adds the missing fields to sign-and-submit responses and refactors the code to eliminate duplication.Context of Change
PR #3125 added augmented response fields to
submit:accepted,applied,broadcast,queued,kept,account_sequence_next,account_sequence_available,open_ledger_cost, andvalidated_ledger_index. These fields were only implemented in the binary blob code path (Submit.cpp), not in the sign-and-submit path (TransactionSign.cpp).To address code review feedback, the implementation has been refactored to extract a shared helper function (
populateAugmentedSubmitFields) that both code paths now use. This eliminates code duplication and ensures both submission modes remain consistent in the future.Bug introduced: v1.5.0-b6 (PR #3125)
Previous behavior: Sign-and-submit returned only legacy fields (
tx_json,tx_blob,engine_result*)Fixed behavior: Both submission modes now return identical augmented fields using a shared implementation
Type of Change
API Impact
Before / After
Before:
After:
Changes
src/xrpld/rpc/detail/TransactionSign.h: Added declaration forpopulateAugmentedSubmitFieldshelper functionsrc/xrpld/rpc/detail/TransactionSign.cpp: AddedpopulateAugmentedSubmitFieldshelper function and updatedtransactionFormatResultImplto use itsrc/xrpld/rpc/handlers/Submit.cpp: Refactored to use the sharedpopulateAugmentedSubmitFieldshelper function, eliminating code duplicationsrc/test/rpc/Submit_test.cpp: Added test coverage verifying augmented fields presence in both sign-and-submit and binary blob modesAPI-CHANGELOG.md: Added changelog entry documenting the bugfix for version 2.5.0Original prompt
This section details on the original issue you should resolve
<issue_title>Augmented submit fields should be included in sign-and-submit mode (Version: 1.5.0-b6)</issue_title>
<issue_description>## Issue Description
The new fields added to the submit method response by #3125 are only provided when you submit a binary blob, not when you provide a JSON and secret.
The fields should be provided in all responses to the
submitcommand.Steps to Reproduce
For a quick offline test:
Start rippled in standalone mode with a fresh ledger.
Sign-and-submit a payment from the genesis account to another address of your choice.
Expected Result
The new fields, such as
applied,broadcast,account_sequence_available, etc. should be included in the response. Here's an example of a similar transaction submitted as binary, with the new fields provided:Actual Result
Actual result matches the old (v1.4.0 and lower) result fields for the submit method:
Environment
Version: rippled version 1.5.0-b6
OS: Arch Linux (latest) x86_64
Self-compiled, Boost 1.72.0. All unit tests pass.
</issue_description>
<agent_instructions>Update the
submitRPC in sign-and-submit mode to add the missing fields. You can use the regularsubmitmode to understand how to fetch and add the fields. Note the issue description and the linked PR for a list of all the missing fields. There should also be tests added for this.One relevant file is probably src/xrpld/rpc/handlers/Submit.cpp.</agent_instructions>
Comments on the Issue (you are...
💡 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.