fix(batch): GetBatchSignerAccounts stops rewriting the batch it reports on - #162
Conversation
…ts on The method returns the root account and the accounts required to sign a batch. It also replaced RawTransactions[i].RawTransaction in the caller's own dictionary with a converted copy: the IEnumerable branch aliases an element that is already a Dictionary, so the assignment landed in the caller's object. Being the gate every batch-signing path reaches through VerifyBatchSubmitter, it did this on every signature. The conversion stays, for reading; only the store-back is gone, and the reason is written next to it so it does not come back. Nothing needed the stored form: Autofill and Submit read RawTransaction from a JsonNode, SignAsBatchPart reads it from the tree it parsed itself, and BatchNormalizer writes its own on a separate path. Two tests, because they answer different questions. One asserts the caller's wrapper still holds the object it was given - restoring the assignment fails it. The other signs the same batch twice, once with the wrapper holding a JsonObject and once an equivalent Dictionary, and requires the same blob; it passes either way, which is the point. It does not detect this change, it pins the property that makes the change safe instead of merely tidy, and it would fail first if the two representations ever stopped serializing alike. The unit suite does not otherwise notice: all 1220 tests pass with the assignment removed, exactly as they did in #157 for the mutation there.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📝 WalkthroughWalkthrough
ChangesBatch signing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change stops batch-signing account discovery from rewriting the caller’s transaction objects while preserving signing behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The implementation removes the store-back mutation in GetBatchSignerAccounts. Added tests verify that the caller’s original object is preserved and that JsonObject and dictionary representations produce identical batch-signing blobs, addressing issue Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
1 participant
Closes #161.
What it does
GetBatchSignerAccountsreturns the root account and the accounts required to sign a batch. It also replacedRawTransactions[i].RawTransactionin the caller's own dictionary with a converted copy — theIEnumerablebranch doeso as Dictionary<string, object>, which aliases an element that is already a dictionary, so the assignment landed in the caller's object. Being the gate every batch-signing path reaches throughVerifyBatchSubmitter, it did this on every signature.The conversion stays, for reading. Only the store-back is gone, with the reason written next to it so it does not come back.
How this was established, in the order the issue asked for
The issue said not to start by removing the assignment, because #157 began from exactly that assumption and it was wrong. So:
1. Removed it and ran the suite. All 1220 tests passed. That proved nothing — it is the same blindness that let #157's first proposal look safe.
2. Built the input where the assignment actually changes something — a wrapper holding a
JsonObjectrather than aDictionary. Signed that batch and an equivalent dictionary-valued one, and compared blobs. Identical, with the assignment removed. So the store-back does not reach the signature.3. Looked for a reader. There is none.
Autofill(Autofill.cs:649) andSubmit(Submit.cs:277) readRawTransactionfrom aJsonNode;SignAsBatchPartreads it from the tree it parsed itself;BatchNormalizerwrites its own on a separate path. Nothing consumes the stored form from the caller's dictionary.Tests
Two, answering different questions:
AreSameJsonObjector an equivalentDictionaryThe second is deliberate. It is not a detector for this diff; it would fail first if the two representations ever stopped serializing alike, which is the assumption step 2 rests on.
Verification
CHANGES.mdentry added under the existing unreleased 11.2.0.0 section. No version bump: 11.2.0.0 is already the pending version and has not shipped.Summary by CodeRabbit
Bug Fixes
Tests