Skip to content

fix: route mixed-case wallet commands correctly - #7

Open
tolga-tom-nook wants to merge 2 commits into
BitgesellOfficial:mainfrom
tolga-tom-nook:fix/mixed-case-command-wallet-routing
Open

fix: route mixed-case wallet commands correctly#7
tolga-tom-nook wants to merge 2 commits into
BitgesellOfficial:mainfrom
tolga-tom-nook:fix/mixed-case-command-wallet-routing

Conversation

@tolga-tom-nook

Copy link
Copy Markdown

Summary

  • Fixes Client#command() multiwallet detection for mixed-case RPC method names.
  • Normalizes method names before looking up multiwallet support, matching the lowercasing already done by Requester.prepare().
  • Adds regression coverage for single and batch mixed-case wallet commands.

Bug / impact

this.methods is keyed by lowercase RPC method names, and Requester.prepare() lowercases the outgoing method. However, Client#command('getBalance') checked this.methods['getBalance'] before preparing the request. That missed multiwallet support and posted to / instead of /wallet/<wallet>, so mixed-case direct command calls could hit the wrong wallet context even though the RPC method sent to Bitgesell Core was valid lowercase getbalance.

RED / GREEN validation

RED on origin/main with the new regression:

0 passing
2 failing
AssertionError: expected '/' to be '/wallet/wallet.dat'

GREEN after fix:

./node_modules/.bin/mocha test/command_wallet_routing_test.js --timeout 20000 --require should
2 passing

./node_modules/.bin/eslint src/index.js test/command_wallet_routing_test.js
git diff --check

Bounty context

This is submitted for the Bitgesell improvement / PR bounty program:

Payout address if approved: EVM/USDT-compatible 0x4a76c7E64C08cF29B59eFC640b4ada97A270d428.

Assisted by Hermes Agent.

@tolga-tom-nook

Copy link
Copy Markdown
Author

Pushed a follow-up test hardening commit (3e8cb9a) to make the mixed-case wallet-routing proof harder to regress:

  • covers mixed-case batch routing when only one batch entry is wallet-scoped (getBlockHash + getBalance still posts to /wallet/wallet.dat)
  • covers the negative path so mixed-case non-wallet commands (getBlockHash) continue posting to /

Validation rerun locally:

  • ./node_modules/.bin/mocha test/command_wallet_routing_test.js --timeout 20000 --require should → 4 passing
  • ./node_modules/.bin/eslint src/index.js test/command_wallet_routing_test.js
  • git diff --check

@MyTH-zyxeon

Copy link
Copy Markdown

Maintainers - this looks like another narrow, reviewer-friendly Bitgesell #81 slice.

What appears concretely useful here:

  • Root cause is crisp: Client#command() checks multiwallet support before the request path normalizes mixed-case RPC method names, so calls like getBalance can hit / instead of /wallet/<wallet>.
  • Scope stays small: routing lookup alignment in src/index.js plus focused regression coverage, without widening into transport or wallet semantics changes.
  • The RED/GREEN proof in the PR body is easy to reason about because it targets the exact wrong-path symptom.

Suggested acceptance checks:

  • single-command mixed-case wallet calls route to /wallet/<wallet> instead of /.
  • batch mixed-case wallet commands keep the same wallet routing guarantee.
  • already-lowercase wallet methods still behave unchanged.
  • non-wallet or unknown RPC methods still follow the repo's existing request path logic and are not accidentally reclassified.

If you want a stricter follow-up before merge, the most useful extra proof would be one explicit unchanged-case control (already-lowercase getbalance) alongside the mixed-case regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants