Skip to content

Conversation

@DariusParvin
Copy link
Contributor

@DariusParvin DariusParvin commented Nov 25, 2025

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist

  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have tested my changes running a local network, and they work as expected
  • I have performed a self-review
  • I have commented my code, particularly in hard-to-understand areas
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed
  • I have added assignees and reviewers to this pull request
  • I have added the PR to the project board or linked it to an issue from the board

Summary by CodeRabbit

  • New Features

    • Added a new RPC endpoint to retrieve public keys by multisig ID. The endpoint returns the requested public key in hex format along with the corresponding ID.
  • Tests

    • Added test coverage for the new public key retrieval functionality.

✏️ Tip: You can customize this high-level summary in your review settings.

@DariusParvin DariusParvin self-assigned this Nov 25, 2025
@coderabbitai
Copy link

coderabbitai bot commented Nov 25, 2025

Walkthrough

A new RPC endpoint GetPublicKeyById is added to the BtcServer service, enabling clients to retrieve hex-encoded public keys by multisig ID. The implementation introduces a shared helper function for key loading and refactors the existing legacy key retrieval to use it.

Changes

Cohort / File(s) Summary
Protobuf Definitions
bin/btc-server/proto/btc_server.proto
Added new RPC method GetPublicKeyById to BtcServer service with corresponding request/response message types: GetPublicKeyByIdRequest (multisig_id: uint32) and GetPublicKeyByIdResponse (publickey: string, multisig_id: uint32).
Generated Client Code
bin/btc-server/client/src/btc_server.rs
Generated client request/response message structs and added async client method get_public_key_by_id with proper gRPC method wiring and readiness checks.
Generated Server Code
bin/btc-server/src/rpc/btc_server.rs
Generated server trait method get_public_key_by_id and service routing for the new RPC endpoint, including UnaryService handler and dispatch wiring.
Server Implementation
bin/btc-server/src/bin/main.rs
Added private helper load_public_key(multisig_id) to load and serialize public keys, refactored existing get_public_key to use the helper, and implemented new public RPC handler get_public_key_by_id. Includes unit test for new endpoint.

Sequence Diagram

sequenceDiagram
    participant Client
    participant BtcServerClient
    participant BtcServer
    participant App
    participant Database

    Client->>BtcServerClient: get_public_key_by_id(multisig_id)
    BtcServerClient->>BtcServer: tonic::Request<GetPublicKeyByIdRequest>
    BtcServer->>App: get_public_key_by_id(req)
    App->>Database: fetch public key package by multisig_id
    Database-->>App: public key package
    App->>App: serialize to hex string
    App-->>BtcServer: GetPublicKeyByIdResponse (publickey, multisig_id)
    BtcServer-->>BtcServerClient: tonic::Response<GetPublicKeyByIdResponse>
    BtcServerClient-->>Client: (publickey, multisig_id)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Key areas requiring attention:
    • Verify that load_public_key correctly handles error cases and matches expected database behavior (get_public_key_package_by_id)
    • Confirm the refactoring of existing get_public_key to use the new helper maintains backward compatibility and returns identical results
    • Validate the unit test coverage for the new endpoint and edge cases (e.g., invalid multisig_id)
    • Review consistency between protobuf field tags and generated message struct definitions

Possibly related PRs

  • Macbeth #1029: Introduces multi-key database APIs (get_public_key_package_by_id, LEGACY_MULTISIG_ID, migration) that this PR depends on for the load_public_key implementation

Suggested reviewers

  • lamafab
  • scottmillner
  • rwlockbg

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(btc-server): add GetPublicKeyById gRPC for multi-key support' clearly and specifically describes the main change: adding a new gRPC endpoint for multi-key functionality.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/multi-key-endpoints

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

@DariusParvin DariusParvin force-pushed the feat/multi-key-endpoints branch from d3d73c0 to 654440b Compare November 25, 2025 20:56
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
bin/btc-server/src/bin/main.rs (1)

1879-1898: Public key handlers correctly share logic and preserve legacy behavior

get_public_key now delegates to load_public_key(LEGACY_MULTISIG_ID) and get_public_key_by_id simply threads through the requested id, which keeps error behavior consistent and avoids duplication. You might optionally add a small test that get_public_key_by_id with a non‑existent multisig_id returns the expected InvalidArgument/Missing key package error, mirroring the existing get_public_key negative test.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5992b8c and 654440b.

📒 Files selected for processing (4)
  • bin/btc-server/client/src/btc_server.rs (2 hunks)
  • bin/btc-server/proto/btc_server.proto (2 hunks)
  • bin/btc-server/src/bin/main.rs (4 hunks)
  • bin/btc-server/src/rpc/btc_server.rs (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
bin/btc-server/client/src/btc_server.rs (2)
bin/btc-server/src/bin/main.rs (4)
  • get_public_key_by_id (1889-1898)
  • req (788-817)
  • req (1080-1090)
  • new (351-560)
bin/btc-server/src/rpc/btc_server.rs (2)
  • get_public_key_by_id (393-399)
  • new (520-522)
bin/btc-server/src/rpc/btc_server.rs (2)
bin/btc-server/client/src/btc_server.rs (2)
  • get_public_key_by_id (551-574)
  • new (372-375)
bin/btc-server/src/bin/main.rs (4)
  • get_public_key_by_id (1889-1898)
  • new (351-560)
  • req (788-817)
  • req (1080-1090)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build
🔇 Additional comments (5)
bin/btc-server/proto/btc_server.proto (1)

20-21: Proto definition for GetPublicKeyById is consistent and well-scoped

RPC signature and request/response messages line up cleanly with existing GetPublicKey and the Rust types used downstream (u32 id, hex-encoded key). No issues from a schema or compatibility standpoint.

Also applies to: 185-193

bin/btc-server/client/src/btc_server.rs (1)

159-171: Client wiring for GetPublicKeyById matches proto and existing patterns

The new request/response types and get_public_key_by_id client method follow the established tonic/prost pattern and use the correct service path. Nothing to change here.

Also applies to: 551-574

bin/btc-server/src/bin/main.rs (2)

26-26: Centralized load_public_key helper is good; confirm DB multi-key migration semantics

The load_public_key helper cleanly encapsulates lookup-by-id plus hex serialization and reuses the existing ToStatus flow and badarg!("Missing key package") semantics. The only thing to double‑check is that the DB layer guarantees get_public_key_package_by_id(LEGACY_MULTISIG_ID) is populated whenever DKG completes (e.g., via set_pubkey_package and/or migrate_legacy_key_package), so legacy flows still see the aggregated key via the new helper.

Also applies to: 676-685


2657-2677: New test_get_public_key_by_id_success accurately exercises the happy path

The test seeds a pubkey package under a specific multisig_id and verifies both the id echo and the hex‑encoded key match the stored package, which is exactly what this endpoint promises.

bin/btc-server/src/rpc/btc_server.rs (1)

159-171: Server-side GetPublicKeyById wiring is consistent with existing RPCs

The new messages, trait method, and /btc_server.BtcServer/GetPublicKeyById route follow the established tonic server patterns and match the proto/client definitions; no issues spotted.

Also applies to: 393-399, 809-854

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.

2 participants