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 78.0% 78.1% +0.1%
=========================================
Files 789 790 +1
Lines 66954 67162 +208
Branches 8108 8105 -3
=========================================
+ Hits 52218 52434 +216
+ Misses 14736 14728 -8
... and 8 files with indirect coverage changes |
| struct ApplyResult | ||
| { | ||
| TER ter; | ||
| bool applied; |
There was a problem hiding this comment.
Please add default values: false / tesSuccess looks good
There was a problem hiding this comment.
I disagree with this - it feels more likely that bugs may be accidentally introduced if there are default values, due to forgetting to initialize them correctly.
There was a problem hiding this comment.
What the difference between bugs with initialized and bugs with randomly initialized values? You can't distinguish the values. But with initialized values you can save some code space.
There was a problem hiding this comment.
My understanding of structs is that it will throw a compiler error if you don't specify them when constructing them. Is that incorrect?
src/test/rpc/Simulate_test.cpp
Outdated
| @@ -630,7 +630,8 @@ class Simulate_test : public beast::unit_test::suite | |||
| auto validateOutput = [&](Json::Value const& resp, | |||
| Json::Value const& tx) { | |||
| auto result = resp[jss::result]; | |||
| checkBasicReturnValidity(result, tx, 5, "20"); | |||
| checkBasicReturnValidity( | |||
| result, tx, 5, env.current()->fees().base * 2); | |||
There was a problem hiding this comment.
Please change 5 -> seq()
src/test/rpc/Simulate_test.cpp
Outdated
| [&](Json::Value const& resp, Json::Value const& tx) { | ||
| auto result = resp[jss::result]; | ||
| checkBasicReturnValidity( | ||
| result, tx, 3, env.current()->fees().base); |
There was a problem hiding this comment.
Please change 3 to seq()
src/test/rpc/Simulate_test.cpp
Outdated
| tx[sfDomain] = newDomain; | ||
|
|
||
| // test with autofill | ||
| testTx(env, tx, testSimulation, 3); |
There was a problem hiding this comment.
last parameter must be bool
src/test/rpc/Simulate_test.cpp
Outdated
| Json::Value const& tx) { | ||
| auto result = resp[jss::result]; | ||
| checkBasicReturnValidity( | ||
| result, tx, 5, env.current()->fees().base * 2); |
There was a problem hiding this comment.
Please change 5 to seq()
src/test/rpc/Simulate_test.cpp
Outdated
| else | ||
| { | ||
| metadata = result[jss::meta]; | ||
| } |
There was a problem hiding this comment.
Please add new line after bracket
src/test/rpc/Simulate_test.cpp
Outdated
| Json::Value const& tx) { | ||
| auto result = resp[jss::result]; | ||
| checkBasicReturnValidity( | ||
| result, tx, 4, env.current()->fees().base); |
There was a problem hiding this comment.
Please change 4 -> seq() value / const
| { | ||
| TER ter; | ||
| bool applied; | ||
| std::optional<TxMeta> metadata = std::nullopt; |
There was a problem hiding this comment.
This initialization is unnecessary as default constructor will do the same.
| if (!tx_json.isMember(jss::TransactionType)) | ||
| { | ||
| return RPC::missing_field_error("tx.TransactionType"); | ||
| } |
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| return jvResult; | ||
| } | ||
|
|
||
| std::shared_ptr<STTx const> stpTrans; |
There was a problem hiding this comment.
what stp is for? "stop"? could you please rename it
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| } | ||
|
|
||
| std::string reason; | ||
| auto tpTrans = std::make_shared<Transaction>(stpTrans, reason, context.app); |
There was a problem hiding this comment.
tpTrans is unclear name, could you please rename it
There was a problem hiding this comment.
Was due to copy-paste, fixed in 0bbf589
| namespace ripple { | ||
|
|
||
| std::optional<Json::Value> | ||
| autofillTx(Json::Value& tx_json, RPC::JsonContext& context) |
There was a problem hiding this comment.
Functions that are not exported - please make them static or put into anonymous namespace.
| namespace ripple { | ||
|
|
||
| std::optional<Json::Value> | ||
| autofillTx(Json::Value& tx_json, RPC::JsonContext& context) |
There was a problem hiding this comment.
This function is too big and can be divided into smaller functions. Please consider not to create functions more than ~50 lines without good reason.
There was a problem hiding this comment.
Made some improvements in 0bbf589
| // binary: <bool> | ||
| // } | ||
| Json::Value | ||
| doSimulate(RPC::JsonContext& context) |
There was a problem hiding this comment.
This function is too big, please divide it.
There was a problem hiding this comment.
I tried making some improvements, but it only made it more convoluted.
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| // Convert the TER to human-readable values | ||
| std::string sToken; | ||
| std::string sHuman; | ||
| transResultInfo(result.ter, sToken, sHuman); |
There was a problem hiding this comment.
Please add some actions if transResultInfo return false. Like return error or make some default error.
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| context.params.get(jss::binary, false).asBool(); | ||
|
|
||
| // Convert the TER to human-readable values | ||
| std::string sToken; |
There was a problem hiding this comment.
Please don't use Hungarian notation, we are moving out from it.
There was a problem hiding this comment.
Fixed in 0bbf589 - was copy-pasted
src/xrpld/rpc/handlers/Simulate.cpp
Outdated
| { | ||
| static const std::string alternateSuccessMessage = | ||
| "The simulated transaction would have been applied."; | ||
| jvResult[jss::engine_result_message] = alternateSuccessMessage; |
There was a problem hiding this comment.
Why not just
jvResult[jss::engine_result_message] = "The simulated transaction would have been applied."; ?
| jvResult[jss::meta] = | ||
| result.metadata->getJson(JsonOptions::none); | ||
| } | ||
| } |
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.