fix: Assorted AI comments#7569
Conversation
|
|
||
| static std::expected<uint256, json::Value> | ||
| parseTicket( | ||
| parseSponsorship( |
There was a problem hiding this comment.
This diff is hard to read, I just properly alphabetized the location of parseSponsorship
| } | ||
|
|
||
| if (deltaSponsoredObjectOwnerCount_ != deltaSponsoredOwnerCount_) | ||
| if (invalidOwnerCountLessThanSponsoredOwnerCount_ > 0) |
There was a problem hiding this comment.
This diff is hard to read, I just swapped the two checks
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR aligns the ledger_entry RPC parsing with the correct parameter schemas for Sponsorship/Ticket/Vault entries, adds coverage for Sponsorship ledger_entry requests, and adjusts an invariant check ordering/message path so the associated tests pass consistently.
Changes:
- Fix
ledger_entryparser dispatch by ensuringparseSponsorship,parseTicket, andparseVaulteach parse the appropriate fields and compute the correct ledger key. - Add
ledger_entryRPC tests covering Sponsorship lookup by(sponsor, sponsee)and by index, including malformed-field cases. - Reorder Sponsorship invariant finalize checks and tweak a runtime error string for clarity.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/xrpld/rpc/handlers/ledger/LedgerEntry.cpp |
Corrects the per-ledger-entry parser functions so Sponsorship/Ticket/Vault requests interpret the right JSON fields and derive the right keylets. |
src/test/rpc/LedgerEntry_test.cpp |
Adds Sponsorship ledger_entry tests and extends field-type mappings used by malformed-input test scaffolding. |
src/test/app/Invariants_test.cpp |
Ensures the Sponsorship invariant test is executed as part of the invariants test suite run. |
src/libxrpl/tx/invariants/SponsorshipInvariant.cpp |
Reorders invariant failure checks to match intended precedence and test expectations. |
src/libxrpl/ledger/helpers/NFTokenHelpers.cpp |
Clarifies an exception message used when repair is impossible due to a link inconsistency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## xrplf/sponsor #7569 +/- ##
=============================================
Coverage 82.2% 82.2%
=============================================
Files 1016 1016
Lines 78401 78401
Branches 9012 9002 -10
=============================================
+ Hits 64440 64454 +14
+ Misses 13952 13938 -14
Partials 9 9
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| if (deltaSponsoredObjectOwnerCount_ != deltaSponsoredOwnerCount_) | ||
| if (invalidOwnerCountLessThanSponsoredOwnerCount_ > 0) |
There was a problem hiding this comment.
GetANameThatNotSoLongOrMaybe2Instead
| jvParams[jss::sponsorship][jss::sponsor] = alice.human(); | ||
| jvParams[jss::sponsorship][jss::sponsee] = bob.human(); | ||
| jvParams[jss::ledger_hash] = ledgerHash; | ||
| json::Value const jrr = |
| json::Value jvParams; | ||
| jvParams[jss::sponsorship] = sponsorshipIndex; | ||
| jvParams[jss::ledger_hash] = ledgerHash; | ||
| json::Value const jrr = |
| env(sponsor::set(alice, 0), sponsor::SponseeAcc(bob)); | ||
| env.close(); | ||
| std::string const ledgerHash{to_string(env.closed()->header().hash)}; | ||
| std::string sponsorshipIndex; |
There was a problem hiding this comment.
It is cleaner not to carry mutable state (sponsorshipIndex) between blocks.
It is currently being set in block 1, and being reused in block 2.
We can initialize it with the value, something like: auto const sponsorshipIndex = to_string(keylet::sponsor(alice.id(), bob.id()).key);
and verify it in both block 1 and block 2.
High Level Overview of Change
This PR:
ledger_entrytestsTxFlags.hchanges (they were a bit overengineered previously, since they don't need to be inserver_definitions)Context of Change
API Impact
N/A