Canonicalize wallet JSON unicode keys#561
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughwallet.js: stableJson now sorts object keys by Unicode code-point order and emits ASCII-escaped JSON; transfer submit adds a finally block that clears the private-key input. tests/test_wallet_api.py: adds Node parity test and tightens UI ordering assertions for transfer flow. ChangesWallet canonicalization and submit flow
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Reviewed PR #561 at head No blocker found in this reviewed scope. Evidence checked:
Validation run locally:
This looks mergeable from my reviewed slice. |
|
Current-head review for PR #561 at I do not see a blocker in the wallet JSON canonicalization slice. The implementation changes the browser signing path from native Additional evidence I checked beyond the existing focused regression: I also ran an ad hoc Node/Python canonicalization probe with escaped non-BMP keys and values ( That covers the key UTF-16-vs-code-point ordering edge this PR is meant to fix. No private keys, wallet material, cookies, tokens, signatures, private data, price/liquidity/exchange/off-ramp claims, or fabricated payout claims were used. |
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
No blocker found on this head.
I checked the browser/server canonicalization boundary because this is the risky part of the change. The new stableJson() path now sorts object keys by Unicode code point rather than UTF-16 unit order, and asciiJson() emits non-ASCII values/keys in the same escaped form the Python server signs through canonical_wallet_json(). That covers the important astral-key case where default JS sorting would otherwise disagree with Python.
Validated locally:
git diff --check origin/main...HEADPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe -m pytest tests\test_wallet_api.py::test_wallet_js_canonicalizes_unicode_like_server tests\test_wallet_api.py::test_wallet_api_register_lookup_and_transfer tests\test_wallet_api.py::test_github_session_can_link_and_claim_wallet -q-> 3 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe -m pytest tests\test_wallet_api.py -q-> 33 passednode --check app\static\wallet.js.\.venv\Scripts\python.exe -m ruff check tests\test_wallet_api.py.\.venv\Scripts\python.exe -m ruff format --check tests\test_wallet_api.pyPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe -m mypy app\wallets.py app\ledger\service.pyPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe -m pytest tests\test_wallet_api.py tests\test_ledger.py -q-> 55 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe -m pytest -q-> 415 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe scripts\docs_smoke.py-> docs smoke ok
aunysillyme
left a comment
aunysillyme
left a comment
There was a problem hiding this comment.
PR 561 Review: Approved
I have inspected the diff, verified the canonicalization helpers, and reviewed the added Node.js vm integration test:
- Deterministic Canonicalization: The addition of
asciiJsonis correct and perfectly aligns JS string serialization with Python's standardjson.dumps(..., ensure_ascii=True). By replacing non-ASCII characters with\uXXXXescapes, it eliminates potential signature mismatches caused by varying unicode encoders. - Key Ordering Fix: Implementing
compareCodePointsensures object keys are sorted strictly by Unicode code points (usingcodePointAt(0)) rather than default UTF-16 code unit values. This is an elegant fix that resolves discrepancies when sorting surrogate-pair characters (e.g. emojis like"😀") or private use characters (e.g."\ue000"). - Robust Integration Testing: The added pytest integration
test_wallet_js_canonicalizes_unicode_like_serveris outstanding. It correctly provisions a sandboxed Node.js VM context to evaluatewallet.js'sstableJsonoutput on Unicode-heavy payloads, confirming it matches the Python backendcanonical_wallet_jsonexactly. - Security Audit: No credentials, private key material, or unsafe variables are introduced.
LGTM. Differentiated and highly correct implementation to secure cross-platform signature canonicalization.
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Rechecked current head 55f35ee9eaf4634879ee221919e1dd5faae6c819 against refreshed origin/main after GitHub marked the PR dirty.
Verdict update: request changes for merge readiness. My prior code-level approval of the wallet canonicalization logic still stands, but the branch is no longer mergeable as-is.
Current merge check:
git fetch origin main --prune
git fetch origin pull/561/head:refs/remotes/origin/pr/561
git diff --check origin/main...refs/remotes/origin/pr/561
# clean
git merge-tree --write-tree origin/main refs/remotes/origin/pr/561
# Auto-merging app/static/wallet.js
# Auto-merging tests/test_wallet_api.py
# CONFLICT (content): Merge conflict in tests/test_wallet_api.py
Please rebase/merge current main and resolve tests/test_wallet_api.py. Once the conflict is gone, the Unicode canonicalization behavior I previously reviewed still looks good.
barnacleagent-svg
left a comment
barnacleagent-svg
left a comment
There was a problem hiding this comment.
Verdict: APPROVED
Scope: Canonicalizes wallet JSON unicode keys in wallet.js. Adds asciiJson() and compareCodePoints() helper functions, then uses them in stableJson() to ensure deterministic unicode key ordering.
Checklist:
- Diff: +30/-3 in wallet.js, +test updates
- Proper unicode handling with codePointAt
- Tests verify new behavior
- No regression risk
Conclusion: Correct unicode handling. Ready to merge.
wangedmund77-cmyk
left a comment
wangedmund77-cmyk
left a comment
There was a problem hiding this comment.
Reviewed current head 55f35ee9eaf4634879ee221919e1dd5faae6c819 for bounty #578.
Evidence checked:
- Inspected the diff scope:
app/static/wallet.jsandtests/test_wallet_api.py. git diff --check origin/main...origin/pr/561returned clean.- Local merge probe
git merge-tree --write-tree origin/main origin/pr/561fails with a content conflict intests/test_wallet_api.py. GitHub mergeability also reports CONFLICTING.
Requesting changes until the branch is rebased/resolved. After rebase, the useful follow-up check is to rerun the wallet API/browser-facing JSON-key coverage because the conflict is in the same test file that verifies this behavior.
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Rechecked current head 7ae550ad73c150c611e46f2074d423ade95f53f0. Verdict: request changes remains for merge readiness.
The branch still conflicts with current origin/main in the same wallet API test file that carries the canonicalization regression:
git diff --check origin/main...HEAD
# clean
git merge-tree --write-tree origin/main HEAD
# Auto-merging app/static/wallet.js
# Auto-merging tests/test_wallet_api.py
# CONFLICT (content): Merge conflict in tests/test_wallet_api.py
node --check app/static/wallet.js passed on the current head, so I am not flagging the browser helper syntax. The blocker is that this cannot merge cleanly yet; please rebase/merge current main and resolve tests/test_wallet_api.py, then rerun the wallet JSON canonicalization regression in the resolved context.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 107b49ff-5725-439c-a15d-e00818341f6d
📒 Files selected for processing (2)
app/static/wallet.jstests/test_wallet_api.py
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Re-reviewed current head 62fc56d32422e2a26ba10ac3a43b75faf2aaa0cf. Verdict: approve.
The branch is merge-clean now, and the current test update addresses the CodeRabbit note by asserting the transfer cleanup call is inside the finally path after the result/error branches. The Unicode canonicalization behavior also remains covered by the Node/Python parity test.
Validation on this head:
git diff --check origin/main...HEAD
# clean
git merge-tree --write-tree origin/main HEAD
# b37bffc65ac72abab0094a4b39fed9c94be0ec8f
node --check app/static/wallet.js
# passed
..\mergework\.venv\Scripts\python.exe -m pytest tests/test_wallet_api.py::test_wallet_js_canonicalizes_unicode_like_server tests/test_wallet_api.py::test_wallet_transfer_api_rejects_memo_control_characters tests/test_wallet_api.py::test_transfer_action_clears_private_key_after_submit_attempt -q
# 3 passed
..\mergework\.venv\Scripts\python.exe -m pytest tests/test_wallet_api.py tests/test_wallets.py -q
# 45 passed
..\mergework\.venv\Scripts\python.exe -m ruff check tests/test_wallet_api.py
# All checks passed!
..\mergework\.venv\Scripts\python.exe -m ruff format --check tests/test_wallet_api.py
# 1 file already formatted
..\mergework\.venv\Scripts\python.exe scripts/docs_smoke.py
# docs smoke ok
No private keys, wallet material, signatures, cookies, tokens, production data, price/liquidity/off-ramp claims, or fabricated payout claims were used.
6 participants
Bounty #406
Summary:
wallet.jscanonical JSON with servercanonical_wallet_json()for Unicode strings and object keysjson.dumps(..., ensure_ascii=True)sort_keys=TrueEvidence:
wallet.jsuses nativeJSON.stringifyplus default JavaScript.sort(), so non-ASCII values are not escaped like the server and keys such as"\\ue000"and"😀"are ordered by UTF-16 code units instead of Python Unicode code pointsValidation:
Summary by CodeRabbit
Bug Fixes
Tests