fix: reuse Protocol V2 wallet session in allNetwork get-address - #934
Conversation
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude review session: https://claude.ai/code/session_01VBxNAjXsLGp9WWYuFiBSxp |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37770cd3ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Related task: OK-63410
Summary
Speed up Protocol V2 all-network address retrieval by reusing the selected wallet session across nested chain methods and batching silent reads by chain. Recoverable item failures retain the valid results, and individual retries suppress only address notifications already emitted by the initial batch.
Behavior and compatibility
allNetworkGetAddress(non-loop) returns the full result array.allNetworkGetAddressByLoopemits per-item callbacks and a finish callback. Public parameters, result shape, input result ordering and App API selection remain unchanged.[success, failure, success].showOnOneKey, individually from the start to prevent repeated confirmations.CLI hardware measurements
Method: macOS Node CLI, Pro2 over USB, Protocol V2, firmware 1.0.1. Compared base
9fe52e6e9f93cfdcfe5ae4800512a2e97b931408with original PR head1bdedeb879cb1e658e2375bf55eeb774714c3f88in the same SDK process, alternating implementations. All requests usedshowOnOneKey: false; initial connection, unlocking, and wallet selection were excluded. The harness called the actual all-network APIs; the CLI's separatebatch-get-addresscommand does not exercise these APIs.Each row below is the median of 3 runs per implementation. Bundles contain equal numbers of EVM and SOL account paths. “Less time” means
(before - after) / before.All 48 timed calls matched the baseline's ordered results; loop callback order also matched. Hidden-wallet results were distinct from the standard wallet.
Additional single-run comparisons (not medians):
For 12 EVM/SOL addresses, total
DeviceSessionGetcalls fell from 13 to 2 (outer wallet check plus the first nested resume); standard-walletDeviceStatusGetcalls fell from 28 to 6. The 12 address commands remain serialized. For BTC/SUI/Aptos,BatchGetPublickeysadditionally fell from 12 to 3.Measurement boundary: these timings compare the explicitly listed baseline and original PR revisions, before the later error-isolation and notification fixes. The alpha.122 implementation preserves the successful silent batching/session path; 72 simulated V2 differential cases against
680227b20confirmed unchanged commands and final results, with unchanged notifications on successful calls. The final artifact has not been timed on hardware, so the table is not a fresh alpha.122 benchmark. Error fallback adds work and is outside the timing figures.Validation and remaining coverage
onekeybaselinedb702ae94; only the documented progress correction differs.680227b20. Commands, final results, progress and other events match; successful notification order/count match; duplicate notifications from failed silent batches are removed.yarn agent:check --profile commitpassed on Node 22.21.1, including Core (88 suites; 1348 passed / 4 skipped) and all affected package tests/builds. Finalyarn agent:check --profile prpassed (full lint, tests, build and version checks). GitHub CI passed for source commit37770cd3a. One existing Electron BLE reacquire test timed out on the first attempt; its focused rerun and the completed full commit check passed without transport changes.npm release
1.2.2-alpha.122; dist-tag:ok63410.hardware-example.37770cd3ae109751f082715228b6155487b7da56.onekeybaselinedb702ae94(through docs: align SDK developer guides with current API contracts #941), error isolation and notification fixes. Relative to that baseline, this PR changes the original three all-network files and 36 version manifests.ok63410tags, integrity metadata andgitHeadvalues match the source commit above.dist/index.jsmatches the locally validated build byte-for-byte.@onekeyfe/hd-core@1.2.2-alpha.122.ok63410tag. Registry verification confirmslatestandnextremained unchanged for all 25 packages.Further optimization
EVM/SOL still pay per-address device processing time, so further gains require firmware batching or fewer requested paths. BTC/SUI/Aptos already benefit from public-key batching. Changing the App's loop mode changes callback behavior and is outside this PR.