feat: implement opt-in receipt and record query failover#1220
feat: implement opt-in receipt and record query failover#1220rwalworth merged 6 commits intohiero-ledger:mainfrom
Conversation
|
Hey @Adityarya11 👋 thanks for the PR! This comment updates automatically as you push changes -- think of it as your PR's live scoreboard! PR Checks✅ DCO Sign-off -- All commits have valid sign-offs. Nice work! ✅ GPG Signature -- All commits have verified GPG signatures. Locked and loaded! ✅ Merge Conflicts -- No merge conflicts detected. Smooth sailing! ✅ Issue Link -- Linked to #1171 (assigned to you). 🎉 All checks passed! Your PR is ready for review. Great job! |
rwalworth
left a comment
There was a problem hiding this comment.
Thanks for working on this @Adityarya11! The deterministic node sort and the bug fix in getRecord(client, timeout) routing through getRecordQuery() are both good catches.
However, after comparing the implementation against the design proposal, the architecture needs to be restructured before I can approve. The main points are:
- The design specifies a single
allowReceiptNodeFailoverflag that controls both receipt and record queries. Your PR introduces two separate flags. - The node-selection logic should live in
TransactionResponse(insidegetReceiptQuery/getRecordQuery), not inonExecuteoverrides on the query classes. This eliminates the need forsetSubmittingNodeId, theonExecuteoverrides, and theQuery.hvisibility change entirely. - The failover node list doesn't account for transaction-specific node IDs (
[submittingNode, ...restOfTxNodes]), only client network nodes.
I've left detailed comments below. Let me know if you have any questions — happy to help!
a6e705a to
863f594
Compare
rwalworth
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround @Adityarya11! The structure now matches the design proposal nicely.
I left a couple of comments below — one blocking item around the constructor change and a non-blocking suggestion from the Codacy findings. Let me know if you have any questions!
Signed-off-by: Aditya Arya <arya050411@gmail.com>
Signed-off-by: Aditya Arya <arya050411@gmail.com>
Signed-off-by: Aditya Arya <arya050411@gmail.com>
863f594 to
aaab8b6
Compare
Signed-off-by: Aditya Arya <arya050411@gmail.com>
aaab8b6 to
1c9f5ea
Compare
rwalworth
left a comment
There was a problem hiding this comment.
Awesome @Adityarya11, we're looking really good now! We're 99% of the way there, there's just one last thing to address that I missed in the previous review (sorry about that!) - it's a copy-paste fix. We should be good to merge after!
Signed-off-by: Aditya Arya <arya050411@gmail.com>
rwalworth
left a comment
There was a problem hiding this comment.
LGTM @Adityarya11, thanks for sticking with this! Once the workflows pass I will merge.
Description:
Implement opt-in receipt/record query failover to other nodes as described in issue. Implementation includes:
allowReceiptNodeFailoverboolean flag toClient(default:false) that controls both receipt and record query failover, with corresponding getter/setter.TransactionResponse::getReceiptQueryandgetRecordQuery, which accept an optionalClient*parameter and build the complete node list before returning the queryTransactionResponsevia an extended constructor, supporting both explicit-node and default-client-network failover scenarios.getRecord(client, timeout)to route throughgetRecordQueryfor consistent node pinningRelated issue(s):
Fixes #1171
Notes for reviewer:
allowReceiptNodeFailovergoverns both receipt and record queries, per the design proposal.AccountIdand sorted by shard → realm → account number ascending.[submittingNode, ...remainingTransactionNodes], (2) transaction used default client nodes →[submittingNode, ...clientNetworkNodes].getNetwork()returnsunordered_map<string, AccountId>where multiple URLs can map to the sameAccountId; deduplication preserves first occurrence.Checklist