Review and verification

How the independent reviews worked, what they found, and what was fixed.

How a review works

After a milestone is built, a separate review agent that did not write the code reviews the change against its brief. It must exercise failure paths rather than read them: it writes tests that try to break the code, reports each finding as Critical, Important or Minor, and gives a verdict of clean, merge with fixes, or block. The implementer then fixes each finding and records the resolution next to it. For work that handles keys or signs transactions, two reviewers work in parallel without seeing each other's work.

Review 1: M1 to M4

• One reviewer. Verdict: merge with fixes. No Critical findings (simulation only).
• 25 reviewer tests, 19 of them failing at first.
• 7 Important findings, among them: a cancel in the same handler as a submit could abort a run; a partly filled stop-loss could leave a position without a working exit; both bracket exits could fill under latency; two orders could take the same displayed size; a timer set in its own handler was scheduled from the wrong time; two catalog writers could lose each other's updates; an edited manifest time range went unnoticed.
• 8 tested Minor findings, including paths that could escape the catalog root and a Windows name collision.
• All fixed except three documented items. 151 tests passed after the fixes. The fixes were not re-reviewed.

Review 2: M5 part 1 and M6

• Two reviewers in parallel. Reviewer A (engine seam, reconciliation, XRPL venue logic): block, with 3 Critical, 6 Important and 4 Minor findings. Reviewer B (keys, signing, encoding, transport): merge with fixes, with 1 Important and 10 Minor findings.
• Critical: an offer missing from the account's offer list was reported as fully filled; one transport error dropped every pending order; a validated cancel dropped fills taken since the last poll.
• Reviewer B's Important finding: a non-ASCII byte in a wallet file caused a panic that printed most of the secret. Wallet files are now exactly 32 hex digits, decoded byte by byte, never sliced and never echoed.
• Other fixes: sequence resync, definitive-only expiry, kept cancels, whole-lot fills, reconcile hooks for the XRPL venue, HTTPS-only with no redirects, restricted wallet file permissions on Windows, atomic wallet writes.
• Both reviewers were limited to read-only testnet calls, so failure paths were exercised with a scripted mock built on real testnet response shapes. Reviewer A recorded that it was insufficient for live failure paths for that reason.
• 211 tests passed after the fixes.

Re-review of the fixes

• A third independent reviewer checked only the fix diff. Verdict: block, with 1 Critical, 3 Important and 5 Minor findings.
• It judged all five test changes made during the fixes to be fair: each mock now answers the way the ledger server does, and none hides a defect.
• Critical: a reconciled offer that was not a whole number of lots could leave a traded lot unreported. Fixed: the ledger amount is tracked as it is and fills are capped at the order size.
• Important: a repeated reconcile could double-follow an offer; a refused reconcile could leave the venue following offers the engine never imported; a cancel intent could be lost on one transient error. All fixed.
• 229 tests passed after the fixes, with lint and format clean.

Review 3: token tools (task 7)

• Two reviewers in parallel. Reviewer A (transaction encoding): merge with fixes, 4 Minor findings. Reviewer B (command safety): merge with fixes, 1 Important and 3 Minor findings. No Critical findings.
• Important: the AMM create fee came from the server with no upper bound and was not shown in the dry run. Fixed: capped at 2 XRP and stated in the dry run.
• Minor fixes: account flags limited to the three the tools need; currency codes checked inside every issued amount; liquidity-provider tokens refused as pool assets; a tec first reply is followed to the validated result; dry-run tests run with the network cut off.
• 249 tests passed after the fixes, with lint and format clean.

Test changes are recorded

Where a reviewer's mock encoded an old mechanism, it was adapted to the realistic ledger response and marked as a fix-time adaptation, keeping the test's intent. No assertion was weakened silently.

Still open

• Live failure paths on testnet (no transactions were approved for review).
• A cancel-rejected event kind, so a strategy is told while a cancel is being retried.
• Paging of account history past 200 entries.
• The race between queued transactions and the account sequence.
• Saturating arithmetic in display paths of statistics and portfolio.