Skip to content

feat: return shieldedAddresses from /multisigWallet - #34

Draft
tuliomir wants to merge 1 commit into
mainfrom
multisig-shielded-addresses
Draft

feat: return shieldedAddresses from /multisigWallet#34
tuliomir wants to merge 1 commit into
mainfrom
multisig-shielded-addresses

Conversation

@tuliomir

@tuliomir tuliomir commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Note

💤 The rules for generating valid addresses on the multisig wallet are not closed yet. This PR will stay suspended until the actual definitions arrive.

Adds shieldedAddresses to every entry of the /multisigWallet response, matching what /simpleWallet has returned since #22. Consumers of a helper-generated multisig wallet previously had to derive the shielded scan/spend pairs live, at roughly +7.6s per wallet.

This is not a cost a consumer can opt out of. wallet-lib derives scan/spend keys whenever a wallet starts from a root key, with no multisig exemption, so a multisig wallet paid it whether or not the test touched a shielded feature.

How it works

  • src/wallet.service.tsgenerateMultisigWallet now populates shieldedAddresses from each participant's own seed, and MultisigWallet gains the field as optional, mirroring SimpleWallet.
  • src/routes.ts — corrects the handler's documented cost model. It claimed a large multisig only costs the caller; the handler is synchronous, so it blocks every client. The real behaviour and the 16-participant ceiling are tracked in /multisigWallet blocks the event loop and 500s above 16 participants #32.
  • README.md — documents the field on /multisigWallet and why the pairs are per participant.

The pairs are per participant, unlike addresses. The P2SH addresses are shared because they derive from the sorted pubkey set; shielded keys derive from each participant's root key and the redeem script plays no part. Note this uses wallet-lib's compliant deriveChild, matching its shielded branch — not the non-compliant derivation its legacy multisig path uses.

Behavioral change

Additive. The field is optional in the response type, so consumers pinned to a wallet-lib without shielded support are unaffected; they can prefer entry.shieldedAddresses and fall back to whatever they use today.

Acceptance criteria

  • Every entry of /multisigWallet's wallets array carries shieldedAddresses with one pair per BIP32 index, asserted at the wire and not only at the service layer.
  • Each participant's pairs derive from that participant's own seed — two participants share addresses but never shieldedAddresses.
  • bun run check passes.

Closes #31

Summary by CodeRabbit

  • New Features
    • Multisig wallet responses now include participant-specific shielded address pairs.
    • Shielded addresses are derived separately for each participant, while multisig P2SH addresses remain shared.
  • Documentation
    • Expanded multisig wallet documentation to describe shielded address behavior and derivation.
    • Documented synchronous processing limits, including the effective 16-participant ceiling and possible errors for larger requests.

Consumers of a helper-generated multisig wallet had to derive the
shielded scan/spend pairs live, because only /simpleWallet returned
them. wallet-lib derives those keys whenever a wallet starts from a
root key, with no multisig exemption, so the cost was paid on every
multisig wallet start whether or not the test touched a shielded
feature — measured at roughly +7.6s per wallet.

The pairs are per participant, derived from that participant's own
seed. Unlike `addresses`, they are not shared: the P2SH addresses come
from the sorted pubkey set, while shielded keys come from the root key
and the redeem script plays no part. Note this uses wallet-lib's
compliant deriveChild, matching its shielded branch — not the
non-compliant derivation its legacy multisig path uses.

Tests assert at the wire as well as the service layer: the field is
optional on MultisigWallet, so a handler projecting a subset of fields
would otherwise drop it with a green typecheck.

Also corrects the handler's documented cost model, which claimed a
large multisig only costs the caller. The handler is synchronous, so
it blocks every other client; that and the 16-participant ceiling are
tracked in #32.

Closes #31

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 4, 2026 18:18
@tuliomir tuliomir added the enhancement New feature or request label Aug 4, 2026
@tuliomir tuliomir self-assigned this Aug 4, 2026
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

/multisigWallet now returns participant-specific shielded address pairs. Multisig P2SH addresses remain shared. Tests cover response shape and derivation. Documentation describes shielded-address behavior, synchronous derivation, and the 16-participant limit.

Changes

Multisig shielded addresses

Layer / File(s) Summary
Wallet contract and generation
src/wallet.service.ts
MultisigWallet optionally includes shieldedAddresses. Generation derives pairs from each participant’s mnemonic.
Response and derivation validation
__tests__/src/wallet-service.test.ts, __tests__/src/routes.test.ts
Tests validate address shape, participant-specific derivation, shared P2SH addresses, and distinct shielded pairs.
Endpoint and shielded-address documentation
README.md, src/routes.ts
Documentation describes returned shielded pairs, derivation behavior, synchronous blocking, and the 16-participant limit.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant multisigWallet
  participant generateMultisigWallet
  participant generateShieldedAddresses
  Client->>multisigWallet: request multisig wallet
  multisigWallet->>generateMultisigWallet: generate participants
  generateMultisigWallet->>generateShieldedAddresses: derive pairs from each participant seed
  generateShieldedAddresses-->>generateMultisigWallet: participant-specific shielded pairs
  generateMultisigWallet-->>Client: shared P2SH addresses and shielded pairs
Loading

Possibly related issues

Possibly related PRs

Suggested labels: tests

Suggested reviewers: copilot, pedroferreira1

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The implementation satisfies issue #31 by returning optional per-participant shieldedAddresses and adding corresponding tests and documentation.
Out of Scope Changes check ✅ Passed All changes support the linked issue, including implementation, tests, API documentation, and correction of the handler cost documentation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: returning shieldedAddresses from the /multisigWallet endpoint.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch multisig-shielded-addresses

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the /multisigWallet API response so each returned multisig wallet entry includes shieldedAddresses, aligning it with /simpleWallet and removing the need for consumers to derive shielded scan/spend pairs at wallet startup time.

Changes:

  • Add optional shieldedAddresses to the MultisigWallet type and populate it in generateMultisigWallet per participant seed.
  • Update /multisigWallet handler documentation to reflect the synchronous (event-loop blocking) cost model.
  • Document the new field and its per-participant semantics in README.md, and add wire-level + service-level tests asserting the new behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/wallet.service.ts Adds shieldedAddresses to MultisigWallet and populates it per participant in generateMultisigWallet.
src/routes.ts Updates the /multisigWallet handler comment to accurately describe synchronous cost/limits.
README.md Documents shieldedAddresses for /multisigWallet, including why pairs are per participant.
tests/src/wallet-service.test.ts Adds unit tests ensuring multisig shielded pairs are populated and per-seed.
tests/src/routes.test.ts Adds wire-level assertions that /multisigWallet includes shieldedAddresses in the response.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tuliomir tuliomir moved this from Todo to In Progress (Done) in Hathor Network Aug 5, 2026
@tuliomir
tuliomir requested a review from pedroferreira1 August 5, 2026 15:55
@tuliomir tuliomir removed this from Hathor Network Aug 6, 2026
@tuliomir
tuliomir marked this pull request as draft August 6, 2026 16:09
@tuliomir

tuliomir commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

The rules for generating valid addresses on the multisig wallet are not closed yet. This PR will stay suspended until the actual definitions arrive.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/multisigWallet omits shieldedAddresses, forcing consumers into live derivation

2 participants