refactor: Use SeqProxy instead of uint32 for all sequence-based keylets - #7890
Open
mvadari wants to merge 5 commits into
Open
refactor: Use SeqProxy instead of uint32 for all sequence-based keylets#7890mvadari wants to merge 5 commits into
mvadari wants to merge 5 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors sequence-based keylet APIs to accept SeqProxy (instead of std::uint32_t) and updates RPC handlers, transactors, ledger helpers, and tests accordingly, aiming to reduce bugs caused by mixing account sequence numbers and ticket sequences.
Changes:
- Update keylet computation functions (e.g., offer/check/escrow/payChannel/nftokenOffer/vault/loan/permissionedDomain) to take
SeqProxy. - Update core transactors and RPC parsing paths to pass
SeqProxy(including usingSTTx::getSeqProxy()where appropriate). - Update a wide set of unit/integration tests and helper utilities to construct and pass
SeqProxyvalues.
Reviewed changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/xrpld/rpc/handlers/VaultInfo.cpp | Wrap vault sequence parameter as SeqProxy when computing vault keylets. |
| src/xrpld/rpc/handlers/ledger/LedgerEntry.cpp | Use SeqProxy for escrow/offer/loan/vault/permissioned-domain keylet parsing. |
| src/tests/libxrpl/tx/AccountSet.cpp | Update ticket-sequence handling to use SeqProxy and extract .value() where builders still take integers. |
| src/test/rpc/Subscribe_test.cpp | Update NFToken offer keylet derivations to use SeqProxy. |
| src/test/rpc/LedgerEntry_test.cpp | Update check/NFToken offer/paychan/permissioned-domain keylet derivations to use SeqProxy. |
| src/test/rpc/AccountTx_test.cpp | Update paychan/check keylet derivations to use SeqProxy. |
| src/test/rpc/AccountObjects_test.cpp | Update check keylet derivation to use SeqProxy. |
| src/test/jtx/TestHelpers.h | Add SeqProxy include and update getCheckIndex implementation to use SeqProxy. |
| src/test/jtx/impl/vault.cpp | Update vault keylet derivation to use SeqProxy. |
| src/test/jtx/impl/TestHelpers.cpp | Update pay-channel helper to construct a SeqProxy for keylet derivation. |
| src/test/jtx/impl/escrow.cpp | Update escrow read keylet derivation to use SeqProxy. |
| src/test/app/Vault_test.cpp | Update vault/loanBroker/loan keylet derivations to use SeqProxy. |
| src/test/app/Sponsor_test.cpp | Update check/ticket/escrow/vault/loanBroker/loan keylet derivations to use SeqProxy. |
| src/test/app/PermissionedDomains_test.cpp | Update permissioned-domain keylet derivations to use SeqProxy. |
| src/test/app/PermissionedDEX_test.cpp | Update offer keylet derivations to use SeqProxy. |
| src/test/app/PayChan_test.cpp | Update pay-channel keylet derivation to use SeqProxy. |
| src/test/app/Offer_test.cpp | Update offer keylet derivations to use SeqProxy. |
| src/test/app/NFTokenDir_test.cpp | Update NFToken offer keylet derivations to use SeqProxy. |
| src/test/app/NFTokenBurn_test.cpp | Update NFToken offer keylet derivations to use SeqProxy. |
| src/test/app/NFTokenAuth_test.cpp | Update NFToken offer keylet derivations to use SeqProxy. |
| src/test/app/NFToken_test.cpp | Update NFToken offer/check keylet derivations to use SeqProxy. |
| src/test/app/MPToken_test.cpp | Update check/escrow/vault keylet derivations to use SeqProxy. |
| src/test/app/LPTokenTransfer_test.cpp | Update check/NFToken offer keylet derivations to use SeqProxy. |
| src/test/app/LoanBroker_test.cpp | Update vault/loanBroker/loan keylet derivations to use SeqProxy. |
| src/test/app/Loan_test.cpp | Update loanBroker/loan/vault keylet derivations to use SeqProxy. |
| src/test/app/Invariants_test.cpp | Update offer/check/escrow/vault/permissioned-domain keylet derivations to use SeqProxy. |
| src/test/app/Freeze_test.cpp | Update NFToken offer/check keylet derivations to use SeqProxy. |
| src/test/app/FlowMPT_test.cpp | Update offer keylet derivation to use SeqProxy. |
| src/test/app/Flow_test.cpp | Update offer keylet derivation to use SeqProxy. |
| src/test/app/FixNFTokenPageLinks_test.cpp | Update NFToken offer keylet derivations to use SeqProxy. |
| src/test/app/EscrowToken_test.cpp | Update escrow keylet derivations to use SeqProxy. |
| src/test/app/Escrow_test.cpp | Update escrow keylet derivations to use SeqProxy. |
| src/test/app/Check_test.cpp | Update check keylet derivation to use SeqProxy. |
| src/test/app/Batch_test.cpp | Update check/loanBroker/loan keylet derivations to use SeqProxy. |
| src/test/app/AccountDelete_test.cpp | Update offer/ticket/check/paychan keylet derivations to use SeqProxy. |
| src/libxrpl/tx/transactors/vault/VaultCreate.cpp | Use getSeqProxy() for vault keylet derivation and store sequence numeric with .value(). |
| src/libxrpl/tx/transactors/system/TicketCreate.cpp | Update ticket keylet derivation to pass a SeqProxy. |
| src/libxrpl/tx/transactors/permissioned_domain/PermissionedDomainSet.cpp | Wrap domain sequence as SeqProxy for keylet derivation. |
| src/libxrpl/tx/transactors/payment_channel/PaymentChannelCreate.cpp | Use getSeqProxy() for paychan keylet derivation. |
| src/libxrpl/tx/transactors/lending/LoanSet.cpp | Wrap loan sequence as SeqProxy for loan keylet derivation. |
| src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp | Use getSeqProxy() for broker keylet derivation and store numeric with .value(). |
| src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp | Wrap OfferSequence as SeqProxy for escrow keylet lookup. |
| src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp | Use getSeqProxy() for escrow keylet derivation. |
| src/libxrpl/tx/transactors/escrow/EscrowCancel.cpp | Wrap OfferSequence as SeqProxy for escrow keylet lookup. |
| src/libxrpl/tx/transactors/dex/OfferCreate.cpp | Use getSeqProxy() for offer sequence and .value() when storing numeric fields; wrap cancel sequence. |
| src/libxrpl/tx/transactors/dex/OfferCancel.cpp | Wrap offer sequence as SeqProxy for keylet derivation. |
| src/libxrpl/tx/transactors/check/CheckCreate.cpp | Use getSeqProxy() for check keylet derivation while storing numeric sequence. |
| src/libxrpl/protocol/Indexes.cpp | Change sequence-based keylet function signatures to accept SeqProxy and use .value() for hashing. |
| src/libxrpl/ledger/helpers/NFTokenHelpers.cpp | Pass SeqProxy directly into keylet::nftokenOffer. |
| include/xrpl/protocol/SeqProxy.h | Add ticket() factory and arithmetic operators for SeqProxy. |
| include/xrpl/protocol/Indexes.h | Update public keylet function declarations to accept SeqProxy and include SeqProxy.h. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
ximinez
requested changes
Jul 28, 2026
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
bthomee
reviewed
Jul 29, 2026
|
|
||
| Keylet | ||
| permissionedDomain(AccountID const& account, std::uint32_t seq) noexcept; | ||
| permissionedDomain(AccountID const& account, SeqProxy seq) noexcept; |
Collaborator
There was a problem hiding this comment.
Don't forget to delete the getTicketIndex(AccountID const&, std::uint32_t) below. (I can't leave a comment on the exact line).
It is still declared publicly right next to the SeqProxy overload, and it's still used directly by production code: LedgerEntry.cpp::parseTicket calls getTicketIndex(*id, *seq) with the raw uint32, which we wouldn't want.
| * Factory function to return a sequence-based SeqProxy | ||
| * Factory function to return a sequence-based SeqProxy. | ||
| * Outside of tests, this function should only be used for "secondary" transaction sequences, | ||
| * e.g. `sfOfferSequence`, or sequence fields in an existing ledger object. DO NOT use this for |
Collaborator
There was a problem hiding this comment.
Is there any way we can prevent anyone from using it for the "primary" sequence of a transaction?
Claude suggested that if you want to actually close this, the options are: restrict sequence()/ticket() to test builds only (the docstring already says that's their main intended audience) and require production code to go through named, context-specific factories or STTx::getSeqProxy(); or at minimum rename them to something self-flagging like unsafeSequence()/unsafeTicket() so a reviewer's eye catches direct use in a diff.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
This PR refactors any keylet function that uses a sequence to take a
SeqProxyinstead of astd::uint32_t, and all downstream changes.Context of Change
Preventing bugs
API Impact
N/A