Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an end-to-end SignMessage flow: OpenAPI + protobuf RPC, signer client, core and wallet interfaces, gRPC handler, Schnorr signing implementation, and fixture-driven tests for valid and invalid cases. Changes
Sequence DiagramsequenceDiagram
participant Client as gRPC Client
participant Handler as SignerHandler
participant Wallet as Wallet Service
participant Hasher as chainhash.HashB
participant Signer as Signer Key
Client->>Handler: SignMessage(hexMessage)
Handler->>Handler: decode hex -> message
Handler->>Wallet: SignMessage(ctx, message)
Wallet->>Hasher: HashB(message)
Hasher-->>Wallet: hash
Wallet->>Signer: Schnorr.Sign(hash)
Signer-->>Wallet: signature bytes
Wallet-->>Handler: signature bytes
Handler->>Handler: encode hex(signature)
Handler-->>Client: SignMessageResponse{signature}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/arkd-wallet/core/application/wallet/service_test.go (1)
37-46: Mark test helper functions witht.Helper().This improves assertion failure locations by attributing failures to callers instead of helper internals.
♻️ Suggested patch
func loadServiceFixtures(t *testing.T) *serviceFixtures { + t.Helper() data, err := os.ReadFile("fixtures/service_fixtures.json") require.NoError(t, err) @@ func getTestKey(t *testing.T, fixtures *serviceFixtures, keyName string) *btcec.PrivateKey { + t.Helper() for _, k := range fixtures.SignMessageTests.TestKeys {Also applies to: 48-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/arkd-wallet/core/application/wallet/service_test.go` around lines 37 - 46, Add t.Helper() at the start of test helper functions so failures attribute to the caller; specifically, update loadServiceFixtures(t *testing.T) to call t.Helper() as the first statement and do the same for the other helper function defined around lines 48-59 (the other helper's name in the diff) so all assertions inside those helpers point to the test that invoked them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/arkd-wallet/core/application/wallet/service_test.go`:
- Around line 37-46: Add t.Helper() at the start of test helper functions so
failures attribute to the caller; specifically, update loadServiceFixtures(t
*testing.T) to call t.Helper() as the first statement and do the same for the
other helper function defined around lines 48-59 (the other helper's name in the
diff) so all assertions inside those helpers point to the test that invoked
them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62b4605c-d0f2-4235-badb-cbb21d5856e9
⛔ Files ignored due to path filters (3)
api-spec/protobuf/gen/signer/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/signer/v1/service.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/signer/v1/service_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (9)
api-spec/openapi/swagger/signer/v1/service.openapi.jsonapi-spec/protobuf/signer/v1/service.protointernal/core/ports/signer.gointernal/infrastructure/signer/client.gopkg/arkd-wallet/core/application/types.gopkg/arkd-wallet/core/application/wallet/fixtures/service_fixtures.jsonpkg/arkd-wallet/core/application/wallet/service.gopkg/arkd-wallet/core/application/wallet/service_test.gopkg/arkd-wallet/interface/grpc/handlers/signer_handler.go
IsaqueFranklin
left a comment
IsaqueFranklin
left a comment
There was a problem hiding this comment.
Overall it looks very solid!
I ran the unit tests provided and validated the changes on my machine, specifically TestSignMessage and TestSignMessage_ConsistentWithSchnorrVerify and they passed just fine.
Sorry for the nits 😅
cACK.
pkg/arkd-wallet/core/application/wallet/fixtures/service_fixtures.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/arkd-wallet/core/application/wallet/testdata/signmessage_fixtures.json (1)
46-51: Consider removingkey_namefrom invalid fixtures (currently unused).In
pkg/arkd-wallet/core/application/wallet/service_test.go:102-120, invalid cases do not consumetc.KeyName; keeping it here can imply behavior that is not actually tested.♻️ Minimal cleanup diff
--- a/pkg/arkd-wallet/core/application/wallet/testdata/signmessage_fixtures.json +++ b/pkg/arkd-wallet/core/application/wallet/testdata/signmessage_fixtures.json @@ { "name": "no_signer_key", "message_hex": "48656c6c6f20576f726c64", - "key_name": "", "expected_error": "signer key not loaded" }--- a/pkg/arkd-wallet/core/application/wallet/service_test.go +++ b/pkg/arkd-wallet/core/application/wallet/service_test.go @@ type signMessageInvalidCase struct { Name string `json:"name"` MessageHex string `json:"message_hex"` - KeyName string `json:"key_name"` ExpectedError string `json:"expected_error"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/arkd-wallet/core/application/wallet/testdata/signmessage_fixtures.json` around lines 46 - 51, The invalid fixtures include an unused "key_name" field (e.g., the "no_signer_key" entry in the "invalid" array of signmessage_fixtures.json) while the test code in service_test.go does not use tc.KeyName; remove the "key_name" property from all entries in the "invalid" array of signmessage_fixtures.json (or alternatively update the tests to actually consume tc.KeyName), ensuring the fixture shape matches what the test expects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/arkd-wallet/core/application/wallet/testdata/signmessage_fixtures.json`:
- Around line 46-51: The invalid fixtures include an unused "key_name" field
(e.g., the "no_signer_key" entry in the "invalid" array of
signmessage_fixtures.json) while the test code in service_test.go does not use
tc.KeyName; remove the "key_name" property from all entries in the "invalid"
array of signmessage_fixtures.json (or alternatively update the tests to
actually consume tc.KeyName), ensuring the fixture shape matches what the test
expects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0fb86777-da94-46d0-8efa-28ac6c77b952
📒 Files selected for processing (2)
pkg/arkd-wallet/core/application/wallet/service_test.gopkg/arkd-wallet/core/application/wallet/testdata/signmessage_fixtures.json
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/arkd-wallet/core/application/wallet/service_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api-spec/openapi/swagger/signer/v1/service.openapi.json`:
- Around line 227-244: The OpenAPI schemas for SignMessageRequest and
SignMessageResponse currently allow any string but the handler expects
hex-encoded input/output; update the SignMessageRequest.message and
SignMessageResponse.signature properties to declare they are hex strings by
adding a clear description ("hex-encoded string"), a pattern constraint (e.g.
regex for hex characters), and an example, so clients know to send/expect hex;
refer to SignMessageRequest and SignMessageResponse (and the signer handler that
decodes/encodes hex) when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 359650cf-201b-41a9-899d-1d8ec13e52ce
⛔ Files ignored due to path filters (2)
api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/signer/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (4)
api-spec/openapi/swagger/signer/v1/service.openapi.jsonapi-spec/protobuf/signer/v1/service.protointernal/infrastructure/signer/client.gopkg/arkd-wallet/interface/grpc/handlers/signer_handler.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api-spec/protobuf/signer/v1/service.proto
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api-spec/protobuf/signer/v1/service.proto (1)
66-67: Clarify raw message vs pre-hashed input in the request comment.Line 66 currently documents encoding but not input semantics. Please state whether clients send raw message bytes or a pre-hash to avoid signing mismatches across implementations.
✏️ Suggested proto comment tweak
message SignMessageRequest { - // hex-encoded message to sign + // hex-encoded raw message bytes to sign (not a pre-hashed digest) string message = 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-spec/protobuf/signer/v1/service.proto` around lines 66 - 67, Update the comment on the protobuf field "message" in service.proto to explicitly state whether clients must send the raw message bytes or a pre-computed hash/digest and to describe the exact encoding and hashing expectations; e.g., clarify that "message" is a hex-encoded representation of raw message bytes (or alternatively a hex-encoded digest), state which hashing algorithm (e.g., SHA-256) clients should use if a pre-hash is required, and note whether the service will hash the input before signing or expects the provided digest to be signed as-is so implementers avoid mismatches when calling the signer API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api-spec/protobuf/signer/v1/service.proto`:
- Around line 66-67: Update the comment on the protobuf field "message" in
service.proto to explicitly state whether clients must send the raw message
bytes or a pre-computed hash/digest and to describe the exact encoding and
hashing expectations; e.g., clarify that "message" is a hex-encoded
representation of raw message bytes (or alternatively a hex-encoded digest),
state which hashing algorithm (e.g., SHA-256) clients should use if a pre-hash
is required, and note whether the service will hash the input before signing or
expects the provided digest to be signed as-is so implementers avoid mismatches
when calling the signer API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce55538a-ee2f-49cf-831f-527cc6fc8dd2
⛔ Files ignored due to path filters (2)
api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/signer/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (2)
api-spec/openapi/swagger/signer/v1/service.openapi.jsonapi-spec/protobuf/signer/v1/service.proto
🚧 Files skipped from review as they are similar to previous changes (1)
- api-spec/openapi/swagger/signer/v1/service.openapi.json
pkg/arkd-wallet/core/application/wallet/testdata/signmessage_fixtures.json
Outdated
Show resolved
Hide resolved
|
👋 Hey @bitcoin-coder-bob — @louisinger requested changes on this PR on March 14th. Just a nudge to check in on the review feedback when you get a chance! |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Arkana Review — SignMessage RPC
Summary
Adds a new SignMessage RPC to the signer service that signs arbitrary messages with the operator's Schnorr key. Exposes via gRPC and REST (POST /v1/sign-message).
Security
POST /v1/sign-message endpoint accepts any hex-encoded message and signs it with the operator's private key. This is fine if the signer service is only accessible internally (operator-to-signer communication), but could be dangerous if any part of the API surface is exposed to untrusted callers. Worth confirming the signer service is not reachable from client-facing ports.
✅ Message hashing: The implementation correctly hashes the message with chainhash.HashB(message) (double-SHA256) before passing to schnorr.Sign. This prevents signing raw transaction sighashes that an attacker might try to sneak through — the double-SHA256 wrapping ensures the signed digest can't be confused with a transaction signature.
ℹ️ No message length limit: The protobuf pattern ^(?:[0-9a-fA-F]{2})*$ validates hex format but doesn't limit length. For a signing endpoint, this is generally fine since the message is hashed anyway, but a max length would be a sensible defense-in-depth measure against abuse.
Implementation
- Proto definition, generated code, Go handler, and wallet implementation all look correct and consistent.
- Test fixtures cover basic cases including empty message, various key lengths, and the no-key error path.
- Schnorr signature size validation in the response (
^[0-9a-fA-F]{128}$, min/max 128 chars = 64 bytes) is correct.
Nit
service.proto— missing trailing newline at EOF.
Overall clean and well-structured. The main question is ensuring the signer service's network exposure is limited to trusted internal callers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api-spec/protobuf/signer/v1/service.proto`:
- Around line 65-77: The SignMessage client currently hex-decodes the signature
returned by the server (in SignMessage in
internal/infrastructure/signer/client.go) but does not validate the decoded byte
length against the proto contract; after hex decoding the string, add a runtime
check that the decoded signature length == 64 bytes (proto enforces 128 hex
chars => 64 bytes) and return a clear error if not, ensuring any malformed or
short/long signatures are rejected before further use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c19a4e0-591b-40a3-aea3-f95f3aa57080
⛔ Files ignored due to path filters (2)
api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/signer/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (4)
api-spec/openapi/swagger/signer/v1/service.openapi.jsonapi-spec/protobuf/signer/v1/service.protopkg/arkd-wallet/core/application/wallet/service_test.gopkg/arkd-wallet/core/application/wallet/testdata/signmessage_fixtures.json
🔍 Arkana Review — SignMessage RPCOverviewAdds Security Notes1. Signing oracle risk — message domain separation (Medium) Consider adding a tagged hash or domain-separated prefix (e.g. 2. No auth/ACL check mentioned 3. Empty message signing Code Quality
Cross-Repo Impact
VerdictImplementation is clean and well-tested. The domain separation concern is the most important item — without it, this RPC becomes a generic signing oracle over the operator key. |
There was a problem hiding this comment.
🔍 Arkana Review — arkd#954
Summary: Adds a SignMessage RPC to the SignerService. Accepts a hex-encoded message, signs it with Schnorr (via chainhash.HashB then schnorr.Sign), and returns the hex-encoded 64-byte signature.
🔒 Security — Key observations
-
Signing scheme: The message is hashed with
chainhash.HashB(double SHA-256) before being passed toschnorr.Sign. This means the RPC signsSHA256(SHA256(message)), which is Bitcoin-standard. Good — no risk of signing raw transaction hashes that could be replayed as actual transactions, since the hash domain is distinct. -
No authentication boundary visible in this diff — the
SignerServiceis wired through the macaroon auth interceptor (visible in the interceptor chain ininterceptor.go), so this RPC inherits the existing auth requirements. Anyone with a valid macaroon can callSignMessage. This is appropriate since they can already callSignTransaction. -
No message length limit on the input — the proto schema only validates hex format (
^(?:[0-9a-fA-F]{2})*$), not maximum length. Sincechainhash.HashBhandles arbitrary input, this is functionally safe, but a very large payload could be used for memory exhaustion. Consider adding amax_lengthconstraint on the request field (e.g., 1MB). -
Missing
signer key not loadedgRPC status mapping — inwallet/service.go, whenSignerKeyis nil, the error is a plainfmt.Errorf("signer key not loaded"). The gRPC handler insigner_handler.goreturns this raw error. It should probably be wrapped instatus.Errorf(codes.FailedPrecondition, ...)for consistency with other handler methods.
✅ What looks good
- Signature validation is thorough — the infrastructure client (
signerClient) checks both hex decode success and exact 64-byte length on the response. - Test coverage — fixture-driven tests cover normal signing, empty message, 32-byte message, large message, different keys, and missing signer key. Verification round-trips through
schnorr.ParseSignature+Verify. - Proto definition is clean — input/output patterns are properly constrained in the OpenAPI spec.
- gRPC handler hex-decode error properly returns
codes.InvalidArgument.
🟡 Nits
- Missing newline at end of
signer_handler.goandservice.proto— will cause diff noise on next edit. - The OpenAPI spec title is empty in
options.openapi.json— appears to be from the newoptions.protofile in #960, not this PR. Worth checking if that's intentional.
Cross-repo impact
SDK parity: The SignMessage RPC is new. All SDKs (ts-sdk, go-sdk, rust-sdk, swift-sdk) will need to add the corresponding client method. Consider tracking this with an umbrella issue.
Verdict: Solid implementation. The two security nits (input length limit, gRPC status wrapping) are non-blocking but worth addressing.
There was a problem hiding this comment.
🔍 Arkana Review — Post-Update (1 new commit)
New commit: d5c29636 — "check decoded signature is 64 bytes in SignMessage client"
✅ Good — Client-Side Signature Length Validation
The new check in internal/infrastructure/signer/client.go:
if len(sig) != 64 {
return nil, fmt.Errorf("invalid signature length: expected 64 bytes, got %d", len(sig))
}This is good defensive coding. Schnorr signatures are always 64 bytes, and validating this on the client side catches malformed responses from the signer service early, before they propagate further into the signing pipeline.
This complements the OpenAPI spec validation (^[0-9a-fA-F]{128}$ with min/max length 128) which enforces it at the HTTP layer. The Go client check catches issues in the gRPC path too.
Previous Review Notes Still Apply
The overall SignMessage RPC looks clean:
- Message is hex-encoded, validated with
^(?:[0-9a-fA-F]{2})*$regex - Signing uses
chainhash.HashB(message)(SHA-256 hash) thenschnorr.Sign— standard Schnorr signing - Tests cover valid signing, edge cases (empty/large messages), key selection, and missing-signer error
- No key material is leaked in responses
No blocking issues. The PR is in good shape after this fix.
🔍 Arkana Review — Post-Update (1 new commit)New commit: ✅ Good cleanupReplaces the inline This is the right call — using sentinel errors enables callers to reliably match on error type (e.g., No concernsNo behavioral change beyond the error message wording. |
4 participants
requested by: #904 (review)
Summary by CodeRabbit
New Features
Tests