feat: simulate RPC to dry-run a tx (XLS-69d)
#5069
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5069 +/- ##
========================================
Coverage 77.9% 77.9%
========================================
Files 783 784 +1
Lines 66677 66877 +200
Branches 8108 8123 +15
========================================
+ Hits 51921 52100 +179
- Misses 14756 14777 +21
... and 13 files with indirect coverage changes |
intelliot
changed the title
simulate RPC (XLS-69d)simulate RPC to dry-run a tx (XLS-69d)
| @@ -40,7 +40,7 @@ getFailHard(RPC::JsonContext const& context) | |||
| } | |||
|
|
|||
| // { | |||
| // tx_json: <object>, | |||
| // tx_blob: <object>, | |||
There was a problem hiding this comment.
Actually it can take both, please change it to something like [tx_json|tx_blob]: <object>,
|
|
||
| - The `feature` method now has a non-admin mode for users. (It was previously only available to admin connections.) The method returns an updated list of amendments, including their names and other information. ([#4781](https://github.com/XRPLF/rippled/pull/4781)) | ||
| - `book_changes`: If the requested ledger version is not available on this node, a `ledgerNotFound` error is returned and the node does not attempt to acquire the ledger from the p2p network (as with other non-admin RPCs). Admins can still attempt to retrieve old ledgers with the `ledger_request` RPC. |
There was a problem hiding this comment.
I believe this document and changes are outdated, please re-base to the latest develop version and make any changes if need
src/test/app/TxQ_test.cpp
Outdated
| // No need to initialize, since it's about to get set | ||
| bool didApply; | ||
| std::tie(parsed.ter, didApply) = ripple::apply( | ||
| auto result = ripple::apply( |
| @@ -1924,9 +1924,9 @@ applyAndTestResult(jtx::Env& env, OpenView& view, STTx const& tx, bool pass) | |||
| { | |||
| auto res = apply(env.app(), view, tx, ApplyFlags::tapNONE, env.journal); | |||
|
|
||
| { | ||
| // transaction with a higher base fee | ||
| Json::Value req; |
There was a problem hiding this comment.
A bit unclear, maybe request?
There was a problem hiding this comment.
req is used for all the other tests
| BEAST_EXPECT(!RPC::contains_error(result)); | ||
| BEAST_EXPECT( | ||
| req[jss::tx_json].isMember(jss::Fee) && | ||
| req[jss::tx_json][jss::Fee] == 50000000); |
There was a problem hiding this comment.
How this magic number was calculated?
There was a problem hiding this comment.
It is the default owner reserve
There was a problem hiding this comment.
So use it, it can be obtained through env.current()->fees()
| @@ -465,7 +465,7 @@ LedgerMaster::applyHeldTransactions() | |||
| ApplyFlags flags = tapNONE; | |||
| auto const result = | |||
| app_.getTxQ().apply(app_, view, it.second, flags, j); | |||
| if (result.second) | |||
| if (result.applied) | |||
| @@ -29,6 +29,13 @@ class Application; | |||
| class STTx; | |||
| class TxQ; | |||
|
|
|||
| struct TxApplyResult | |||
| { | |||
| TER ter; | |||
There was a problem hiding this comment.
Please use _ for members, like ter_
| @@ -29,6 +29,13 @@ class Application; | |||
| class STTx; | |||
| class TxQ; | |||
|
|
|||
| struct TxApplyResult | |||
There was a problem hiding this comment.
This name is ambiguous. We have apply() from apply.h and doApply() from applySteps.h functions which return TxApplyResult. And applyTransaction() fucntion from apply.h which return ApplyResult
You should rename
ApplyResult - > ApplyTransactionResult
TxApplyResult -> ApplyResult
At least it will be closer to the function names
| @@ -69,6 +69,12 @@ class ApplyContext | |||
| return *view_; | |||
| } | |||
|
|
|||
| ApplyFlags const& | |||
There was a problem hiding this comment.
Please change to ApplyFlags flags() const
| if (!isDryRun) | ||
| { | ||
| // add any new modified nodes to the modification set | ||
| for (auto& mod : newMod) |
| { | ||
| jvRequest[jss::tx_blob] = jvParams[0u].asString(); | ||
| } | ||
| if (jvParams.size() == 2) |
There was a problem hiding this comment.
Please add new line space
| { | ||
| tx[jss::Fee] = "0"; | ||
| } | ||
| if (!tx.isMember(jss::Sequence)) |
There was a problem hiding this comment.
Please make a new line spaces among these if
| @@ -710,6 +709,104 @@ transactionFormatResultImpl(Transaction::pointer tpTrans, unsigned apiVersion) | |||
|
|
|||
| //------------------------------------------------------------------------------ | |||
|
|
|||
| XRPAmount | |||
| getBaseFee(Application const& app, Config const& config, Json::Value tx) | |||
There was a problem hiding this comment.
base fee is too general here. More like getTransactionMinimumBaseFee
| @@ -710,6 +709,104 @@ transactionFormatResultImpl(Transaction::pointer tpTrans, unsigned apiVersion) | |||
|
|
|||
| //------------------------------------------------------------------------------ | |||
|
|
|||
| XRPAmount | |||
| getBaseFee(Application const& app, Config const& config, Json::Value tx) | |||
There was a problem hiding this comment.
tx value modified here, shouldn't it be passed as ref here and through getCurrentFee
| } | ||
|
|
||
| Json::Value | ||
| getCurrentFee( |
There was a problem hiding this comment.
Current fee is to general here. Please consider more specialized name like getTransactionEscalatedFee
| @@ -710,6 +709,104 @@ transactionFormatResultImpl(Transaction::pointer tpTrans, unsigned apiVersion) | |||
|
|
|||
| //------------------------------------------------------------------------------ | |||
|
|
|||
| XRPAmount | |||
| getBaseFee(Application const& app, Config const& config, Json::Value tx) | |||
There was a problem hiding this comment.
This function is not exported, please make it static or put into anonymous namespace.
| @@ -21,6 +21,7 @@ | |||
| #define RIPPLE_TXQ_H_INCLUDED | |||
|
|
|||
| #include <xrpld/app/tx/applySteps.h> | |||
| #include <xrpld/app/tx/detail/Transactor.h> | |||
There was a problem hiding this comment.
TxApplyResult declared in applySteps.h
| @@ -133,6 +133,14 @@ preflight1(PreflightContext const& ctx) | |||
| NotTEC | |||
| preflight2(PreflightContext const& ctx) | |||
| { | |||
| if (ctx.flags & tapDRY_RUN) // simulation | |||
| { | |||
| if (ctx.tx.getSignature().empty()) | |||
There was a problem hiding this comment.
Why not checked for multisign?
| return feeOrError; | ||
| tx_json[jss::Fee] = feeOrError; | ||
| } | ||
| if (!tx_json.isMember(sfSigningPubKey.jsonName)) |
There was a problem hiding this comment.
please add space here and among other c-independent if blocks
mvadari
changed the title
simulate RPC to dry-run a tx (XLS-69d)simulate RPC to dry-run a tx (XLS-69d)
High Level Overview of Change
This PR adds a new API method, titled
simulate, which executes a dry run of a transaction submission.This PR also fixes #5070.
Context of Change
It is useful to take a transaction, simulate execution it in the current ledger, and return the metadata - but not persist the transaction in the ledger. This can be used for testing, analysis, and more.
XLS spec: XRPLF/XRPL-Standards#207
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Test Plan
Testing is still in progress. Unit tests are and will be added.
Current Status
This PR is complete and ready for review. You can build this branch and sync with the network of your choice (including Mainnet). The public is welcome to test and use this code (at your own risk). Next steps are code review and QA testing.