RPC commands understand markers derived from all ledger object types #4361
Conversation
| auto escrow = [&env]( | ||
| Account const& account, | ||
| Account const& to, | ||
| STAmount const& amount) { | ||
| Json::Value jv; | ||
| jv[jss::TransactionType] = jss::EscrowCreate; | ||
| jv[jss::Flags] = tfUniversal; | ||
| jv[jss::Account] = account.human(); | ||
| jv[jss::Destination] = to.human(); | ||
| jv[jss::Amount] = amount.getJson(JsonOptions::none); | ||
| NetClock::time_point finish = env.now() + 1s; | ||
| jv[sfFinishAfter.jsonName] = finish.time_since_epoch().count(); | ||
| return jv; | ||
| }; |
There was a problem hiding this comment.
I'm noticing that there are functions or manual code to create escrows scattered around the test subdirectory. I see them in...
- Escrow_test.cpp,
- AccountDelete_test.cpp,
- LedgerRPC_test.cpp,
- AccountObjects_test.cpp,
- AccountTx_test.cpp, and
- LedgerData_test.cpp
It's probably time we bit the bullet and wrote some common code to build escrows in the test environment. But I don't need to burden this pull request with that change. Just an observation.
There was a problem hiding this comment.
I agree. I actually stole my code from Escrow_test.cpp, I think.
There was a problem hiding this comment.
I can attempt to apply the solve in this PR if you let me know what you want.
There was a problem hiding this comment.
Hi @dangell7, thanks for the offer. I can't give you a precise definition of what I'm looking for, but here's a hand wave.
First, note that there are several different unit test files that locally define functions or lambdas which
- Create escrows,
- Finish escrows, or
- Cancel escrows.
Here's where those functions are defined (locally) in Escrow_test.cpp: https:/XRPLF/rippled/blob/develop/src/test/app/Escrow_test.cpp#L170-L190
Since this functions are only defined locally, they are not available to all of the test files that need to do these escrow operations. So the functions have been copied, pasted, and mutated into different test files. The escrow lambda, on line 548 here in this file, is one such example.
However, if we pulled those functions out of Escrow_test.cpp and put them somewhere they could be used by all the tests, then we can remove a lot of code duplication.
An example you could consider following is the src/test/jtx/check.h and src/test/jtx/impl/check.cpp files. Those files create a check namespace and put all of the check-related functions in there. So in tests the functions are invoked like check::create(...) or check::cash(...). That seems like a pretty good approach for less common facilities like checks and escrows.
But once you've made those functions in the escrow namespace the work has only started. Now you need to go through all of the tests that use escrows and remove all of those local functions and lambdas. Replace calls to those local functions will calls to the new functions you created in the escrow namespace. This may get tricky, because the escrow-related functions that have been copied around may have mutated. So the interfaces of the copied functions may not be uniform across the code base.
Once you've...
- Created the new globally accessible test functions,
- Removed all of the local definitions,
- Replaced calls to those local definitions with calls to the new global functions, and
- The unit tests all pass.
Then you're done.
Does that make sense? It may be more work than you want to take on in your pull request. It may make more sense as a separate pull request.
| auto payChan = [](Account const& account, | ||
| Account const& to, | ||
| STAmount const& amount, | ||
| NetClock::duration const& settleDelay, | ||
| PublicKey const& pk) { | ||
| Json::Value jv; | ||
| jv[jss::TransactionType] = jss::PaymentChannelCreate; | ||
| jv[jss::Flags] = tfUniversal; | ||
| jv[jss::Account] = account.human(); | ||
| jv[jss::Destination] = to.human(); | ||
| jv[jss::Amount] = amount.getJson(JsonOptions::none); | ||
| jv["SettleDelay"] = settleDelay.count(); | ||
| jv["PublicKey"] = strHex(pk.slice()); | ||
| return jv; | ||
| }; |
There was a problem hiding this comment.
The note for escrows, above, applies to payChans as well. They could both be addressed in the same pull request, but not necessarily this pull request.
There was a problem hiding this comment.
I can attempt to apply the solve in this PR if you let me know what you want.
There was a problem hiding this comment.
Hi @dangell7. Thanks for this offer also. The notes here https:/XRPLF/rippled/pull/4361/files#r1109064286 describe what would be done for escrows. An almost identical process could be followed for PayChan. You can think about whether that's a task you would like to take on.
WietseWind
requested review from
RichardAH
and removed request for
WietseWind
* Resolves XRPLF#4340 and XRPLF#4354 * Add unit test that walks all the object types and verifies that all of their indexes can work as a marker.
* Add more NFTokenOffer object test cases * Add comments explaining the method for checking sfDestination
* upstream/develop: Add a unit test for invalid memos (XRPLF#4287)
* upstream/develop: Make NodeToShardRPC a manual test (XRPLF#4379) Update build instructions (XRPLF#4376)
* upstream/develop: Update BUILD.md (XRPLF#4383)
* upstream/develop: Update dependency: grpc (XRPLF#4407) Introduce min/max observers for Number Optimize uint128_t division by 10 within Number.cpp Replace Number division algorithm Include rounding mode in XRPAmount to STAmount conversion. Remove undefined behavior * Taking the negative of a signed negative is UB, but taking the negative of an unsigned is not. Silence warnings Introduce rounding modes for Number: Use Number for IOUAmount and STAmount arithmetic Add tests Add implicit conversion from STAmount to Number Add clip Add conversions between Number, XRPAmount and int64_t AMM Add Number class and associated algorithms
* upstream/develop: Update documented pathfinding configuration defaults: (XRPLF#4409)
* upstream/develop: Rename to fixNonFungibleTokensV1_2 and some cosmetic changes (XRPLF#4419) Only account specified as destination can settle through brokerage: (XRPLF#4399) Prevent brokered sale of NFToken to owner: (XRPLF#4403) Fix 3 issues around NFToken offer acceptance (XRPLF#4380) Allow NFT to be burned when number of offers is greater than 500 (XRPLF#4346) Add fixUnburnableNFToken feature (XRPLF#4391) Change default vote on fixUniversalNumber from yes to no (XRPLF#4414)
* upstream/develop: Resolve a couple of Github Action CI annoyances: (XRPLF#4413)
* upstream/develop: README: Add a few source code starting points (XRPLF#4421)
* upstream/develop: Fix Conan version constraint in workflows (XRPLF#4430) Refactor getTrustedForLedger() (XRPLF#4424) README: Update "Build from source" section (XRPLF#4426)
* upstream/develop: Undo API changes introduced in XRPFees: (XRPLF#4429) Remove recipe for RocksDB and add recipe for Snappy (XRPLF#4431)
* upstream/develop: Set version to 1.10.0-rc4 Rename 'NFT' to 'NFToken' in DisallowIncoming flags (XRPLF#4442) Update Docker.md (XRPLF#4432) Update package building scripts and images to use Conan (XRPLF#4435) Disable duplicate detector: (XRPLF#4438)
@intelliot No, it's not a breaking change. The API is already broken. This fixes it. So, yes, bug fix.
Markers are not intended to be compatible or reusable across different server instances, much less versions. However, it's worth noting that the older rippled servers will provide these markers already - they just can't understand them. |
* upstream/develop: Set version to 1.10.0
* upstream/develop: docs: security bug bounty acknowledgements (XRPLF#4460)
* upstream/develop: Rectify the import paths of boost/iterator: (XRPLF#4293) Allow port numbers be be specified with a colon: (XRPLF#4328) Use <=> operator for base_uint, Issue, and Book: (XRPLF#4411) Reporting Mode: Do not attempt to acquire missing data from peer network (XRPLF#4458)
ximinez
added
the
Passed
High Level Overview of Change
Users are encountering situations where
markers returned by theaccount_linescommand don't work on subsequent commands, returning "Invalid Parameters".Resolves #4340 and #4354
Context of Change
This issue derives from the optimization implemented in Enforce account RPC limits by account objects traversed. Previously, the ledger traversal would find up to
limitaccount lines, and if there were more, the marker would be derived from the key of the next account line. After the change, ledger traversal would consider up tolimitaccount objects of any kind found in the account's directory structure. If there were more, the marker would be derived from the key of the next object, regardless of type.Note that because of this optimization it is expected that
account_linesmay return fewer thanlimitaccount lines, even 0, along with amarkerindicating there are more available.The problem is that this optimization did not update the
RPC::isOwnedByAccounthelper function to handle those other object types. Additionally, the XLS20d implementation addedltNFTOKEN_OFFERledger objects to objects that have been added to the account's directory structure, but did not updateRPC::isOwnedByAccountto be able to handle those objects. Themarkerprovided in the example for #4354 includes the key for anltNFTOKEN_OFFER. When thatmarkeris used on subsequent calls, it is not recognized as valid, and so the request fails.Type of Change
Test Plan
Reproduce the situation described in #4354. It may be necessary to specify
ledger_index75211418, because ledger state may change. With the old code, the second request, using themarkerwill fail. With the new code, it will succeed.