Add DKG Round 2 parsing, NIP-46 support, fix USB device paths#27
Add DKG Round 2 parsing, NIP-46 support, fix USB device paths#27kwsantiago merged 1 commit intomainfrom
Conversation
|
Warning Rate limit exceeded@kwsantiago has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (18)
WalkthroughAdds DKG Round2 and NIP46 parsing/handling and callbacks; introduces nip46 request/response types and helpers; adds get_share_info RPC backed by a new frost_state participants field; updates default serial device paths in scripts/tests and adjusts related test expectations. Changes
Sequence Diagram(s)sequenceDiagram
participant WS as WebSocket Handler
participant Parser as Event Parser
participant Crypto as Crypto Module
participant App as Application Callback
rect rgb(240,248,255)
Note over WS,App: DKG Round2 flow (new)
WS->>Parser: receive event (DKG_ROUND2)
Parser->>Crypto: derive shared secret / decrypt (our_privkey + sender_pubkey)
Crypto-->>Parser: decrypted JSON payload
Parser->>Parser: extract group, sender/recipient indices, share → populate frost_dkg_round2_t
Parser-->>WS: parsed round2 struct
WS->>App: on_dkg_round2(round2, ctx)
App-->>WS: return
end
rect rgb(255,249,240)
Note over WS,App: NIP46 connect flow (new)
WS->>Parser: receive event (NIP46_CONNECT)
Parser->>Crypto: decrypt content (our_privkey + sender_pubkey)
Crypto-->>Parser: decrypted JSON payload
Parser->>Parser: extract id, method, params, sender_pubkey → populate nip46_request_t
Parser-->>WS: parsed request
WS->>App: on_nip46_request(request, ctx)
App-->>WS: return
WS->>Parser: frost_nip46_request_free(request)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
kwsantiago
force-pushed
the
feat/dkg-nip46-usb-fixes
branch
from
d53007e to
5d317fa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @main/nostr_frost.c:
- Around line 1397-1407: Ensure "error" and "result" are treated as mutually
exclusive: if response->error is present add only the error to content_obj and
skip adding response->result; otherwise perform the existing response->result
handling (parse into result_json and add it or fall back to a string). Update
the block handling response->result/response->error to check response->error
first (or explicitly guard adding result with !response->error) and use the
existing result_json/content_obj logic unchanged when allowed.
In @scripts/test_all_rpc.py:
- Around line 80-83: The print on the failure branch uses an f-string with no
placeholders; change the unnecessary f-string print(f" FAIL: no commitment in
response") to a plain string print(" FAIL: no commitment in response") in
scripts/test_all_rpc.py (the failure branch after the "if 'commitment' in
result" check) so the code uses a normal string literal.
🧹 Nitpick comments (3)
main/frost_signer.c (1)
190-207: Consider extracting common logic to reduce duplication.The function shares significant logic with
frost_get_pubkey(lines 172-188):
- Loading state (lines 191-195 vs 173-177)
- Converting pubkey to hex (lines 197-198 vs 179-180)
- Cleanup pattern (line 206 vs 187)
Consider extracting a helper function that loads state and returns the populated structure, then have both functions format their specific JSON responses.
Additionally,
bytes_to_hex(line 102) is duplicated across multiple files (frost_coordinator.c:72-77, frost_dkg.c:45-50). Consider moving it to a shared utility module.main/nostr_frost.c (2)
1020-1032: Potential confusion from dual sources of index fields.The function reads
sender_indexandrecipient_indexfrom both:
- Tags (lines 1028-1032):
participant_index→sender_index,recipient_index→recipient_index- Inner JSON content (lines 1057-1064):
sender_indexandrecipient_indexfieldsIf both sources provide values and they differ, the inner JSON values silently override the tag values. This could lead to confusion or inconsistencies.
Consider either:
- Validating that both sources match when both are present, or
- Documenting which source is authoritative
Also applies to: 1057-1064
1053-1056: Consider validating hex string length before conversion.The
sharefield is converted toencrypted_share(48 bytes), but there's no validation that the hex string has the expected length (96 characters for 48 bytes) before callinghex_to_bytes.While
hex_to_byteslikely handles this gracefully, explicit validation would make the code more robust and provide clearer error handling.🔎 Proposed validation
cJSON *share = cJSON_GetObjectItem(inner, "share"); if (share && cJSON_IsString(share)) { + if (strlen(share->valuestring) != 96) { + cJSON_Delete(inner); + free(decrypted); + cJSON_Delete(root); + return -4; + } hex_to_bytes(share->valuestring, round2->encrypted_share, 48); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
README.mdmain/frost.cmain/frost.hmain/frost_coordinator.cmain/frost_coordinator.hmain/frost_signer.cmain/frost_signer.hmain/main.cmain/nostr_frost.cmain/nostr_frost.hmain/protocol.cmain/protocol.hscripts/monitor_serial.pyscripts/test_all_rpc.pyscripts/test_dkg_full.pyscripts/test_dkg_relay.pytest/hardware/test_hardware.pytest/integration/test_full_flow.sh
✅ Files skipped from review due to trivial changes (1)
- scripts/test_dkg_full.py
🚧 Files skipped from review as they are similar to previous changes (5)
- test/integration/test_full_flow.sh
- main/frost_coordinator.c
- main/nostr_frost.h
- scripts/test_dkg_relay.py
- test/hardware/test_hardware.py
🧰 Additional context used
🧬 Code graph analysis (4)
main/frost_signer.c (3)
main/protocol.c (2)
protocol_error(147-154)protocol_success(138-145)main/frost_coordinator.c (1)
bytes_to_hex(73-78)main/frost.c (1)
frost_free(161-170)
main/main.c (1)
main/frost_signer.c (1)
frost_get_share_info(190-207)
main/frost_signer.h (1)
main/frost_signer.c (1)
frost_get_share_info(190-207)
main/nostr_frost.c (2)
main/frost_dkg.c (2)
hex_to_bytes(35-44)bytes_to_hex(46-51)main/frost_coordinator.c (1)
bytes_to_hex(73-78)
🪛 Ruff (0.14.10)
scripts/test_all_rpc.py
83-83: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ 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 (16)
scripts/monitor_serial.py (1)
7-7: LGTM! Device path update aligns with ESP32 USB CDC.The change from
/dev/ttyUSB0to/dev/ttyACM0correctly reflects ESP32 boards using native USB CDC, and users can still override via--deviceif needed.scripts/test_all_rpc.py (4)
94-96: API contract change handled correctly.The change to use a hardcoded
session_idinstead of extracting it fromcommit_resultcorrectly reflects the updated API wheresession_idis now an explicit input parameter rather than being derived from the commitment response. Thecommit_resultis still appropriately used to control test flow.
127-164: Well-structured test suite.The test execution flow is logical, with proper sequencing of operations (import → commit → sign → delete) and comprehensive result tracking. The test suite effectively validates the updated RPC interface.
7-7: Confirm the device path matches your target hardware configuration.The change from
/dev/ttyUSB0to/dev/ttyACM0indicates the ESP32-S3 is configured to use CDC ACM (native USB serial) instead of a USB-to-serial adapter. Ensure this aligns with your hardware setup, as the correct device path depends on your ESP32-S3's firmware configuration and USB mode.
74-74: Session_id reuse is acceptable for this test. RFC 9591 (FROST protocol) does not mandate session_id uniqueness—it is an implementation detail. Hardcoding test values is a standard practice, and the actual hardware implementation properly generates unique session IDs when needed (seetest/hardware/test_hardware.py).main/frost.h (1)
14-14: LGTM!The new
participantsfield is well-positioned and uses a consistent type with other fields in the struct.main/frost.c (1)
135-135: LGTM!The initialization of the
participantsfield follows the same pattern as other fields and correctly assigns the value frommax_participantsextracted from the serialized share data.main/frost_coordinator.h (1)
24-25: LGTM!The new callback typedefs and struct members follow the established pattern and are well-integrated with the existing callback structure. The naming is clear and consistent.
Also applies to: 31-32
main/nostr_frost.c (3)
1435-1441: LGTM!The cleanup function properly frees the allocated
paramsstring and resets the associated fields. Consistent with the pattern used infrost_sign_request_free.
1345-1349: No action needed—NIP46 requires encryption per specification.NIP-46 (Nostr Connect) events must have encrypted content per the specification ("the event content MUST be an encrypted JSON-RPC-like payload"). The strict error return when decryption fails is correct and differs intentionally from
frost_parse_dkg_round1_event, which lacks this encryption requirement.
1046-1049: Inconsistent decryption handling compared tofrost_parse_dkg_round1_event.Both functions always encrypt their messages during creation, but they differ during parsing.
frost_parse_dkg_round1_event(line 881) falls back to unencrypted content if decryption fails, whilefrost_parse_dkg_round2_event(lines 1046-1049) returns error-3immediately on decryption failure.If DKG Round2 messages must always be encrypted per protocol (unlike Round1), this is correct. Otherwise, clarify whether Round2 should also support the fallback pattern for consistency.
README.md (1)
176-176: LGTM! Documentation aligns with implementation.The new RPC method documentation is clear and accurately describes the functionality implemented in
frost_signer.c.main/main.c (1)
90-92: LGTM! Handler follows established pattern.The new case for
RPC_METHOD_GET_SHARE_INFOcorrectly routes tofrost_get_share_infoand follows the same pattern as the existingGET_SHARE_PUBKEYhandler.main/protocol.c (1)
9-9: LGTM! Method parsing is correct and consistent.The new method string parsing for "get_share_info" follows the established pattern and is logically positioned with other share-related methods.
main/frost_signer.h (1)
10-10: LGTM! Function signature is appropriate and consistent.The new function declaration for
frost_get_share_infomatches the pattern offrost_get_pubkeyand has an appropriate signature for the RPC handler use case.main/protocol.h (1)
27-27: LGTM! Enum member properly positioned.The new
RPC_METHOD_GET_SHARE_INFOenum member is logically placed afterGET_SHARE_PUBKEY, maintaining grouping of related methods while preservingRPC_METHOD_UNKNOWNas the sentinel value.
kwsantiago
force-pushed
the
feat/dkg-nip46-usb-fixes
branch
from
5d317fa to
ddc49eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @main/nostr_frost.c:
- Around line 1397-1406: The response handling block (checking response->error
and response->result) lacks a validation for the case when both are NULL; add a
guard after constructing content_obj to detect if both response->error and
response->result are NULL and produce a valid NIP-46 error (e.g., set "error" in
content_obj to a descriptive message or a standard NIP-46 error string) so every
response contains either "error" or "result"; reference symbols: response,
response->error, response->result, and content_obj.
🧹 Nitpick comments (1)
main/frost_signer.c (1)
190-207: LGTM! Function correctly retrieves and formats share info.The implementation properly loads the frost state, formats the response JSON, and cleans up resources. Buffer size (192 bytes) is adequate for the JSON payload.
💡 Optional: Consider extracting common load-and-format logic
Lines 191-198 duplicate logic from
frost_get_pubkey(lines 173-180). If more similar accessors are added, consider extracting a helper:static int load_and_get_pubkey_hex(const char *group, frost_state_t *state, char *pubkey_hex) { if (load_frost_state(state, group) != 0) { return -1; } bytes_to_hex(state->group_pubkey, sizeof(state->group_pubkey), pubkey_hex); return 0; }This would centralize the load-convert pattern used by both
frost_get_pubkeyandfrost_get_share_info.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
README.mdmain/frost.cmain/frost.hmain/frost_coordinator.cmain/frost_coordinator.hmain/frost_signer.cmain/frost_signer.hmain/main.cmain/nostr_frost.cmain/nostr_frost.hmain/protocol.cmain/protocol.hscripts/monitor_serial.pyscripts/test_all_rpc.pyscripts/test_dkg_full.pyscripts/test_dkg_relay.pytest/hardware/test_hardware.pytest/integration/test_full_flow.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- test/integration/test_full_flow.sh
- main/frost.h
- scripts/test_dkg_full.py
- main/frost_coordinator.h
- main/protocol.h
- main/frost.c
🧰 Additional context used
🧬 Code graph analysis (5)
main/frost_coordinator.c (1)
main/nostr_frost.c (3)
frost_parse_dkg_round2_event(991-1074)frost_parse_nip46_event(1324-1376)frost_nip46_request_free(1434-1440)
main/frost_signer.h (1)
main/frost_signer.c (1)
frost_get_share_info(190-207)
main/nostr_frost.c (2)
main/frost_dkg.c (2)
hex_to_bytes(35-44)bytes_to_hex(46-51)main/frost_coordinator.c (1)
bytes_to_hex(73-78)
main/main.c (1)
main/frost_signer.c (1)
frost_get_share_info(190-207)
main/frost_signer.c (2)
main/protocol.c (2)
protocol_error(147-154)protocol_success(138-145)main/frost_coordinator.c (1)
bytes_to_hex(73-78)
⏰ 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 (17)
scripts/monitor_serial.py (1)
7-7: LGTM!The default device path update to
/dev/ttyACM0is appropriate for ESP32-S3's native USB CDC interface, which enumerates as an ACM device rather than a USB-to-serial adapter.test/hardware/test_hardware.py (1)
8-8: LGTM!The default device path update is consistent with the other scripts in this PR while preserving the environment variable override for flexibility.
scripts/test_dkg_relay.py (2)
23-23: LGTM!Device path update is consistent with the broader PR changes.
174-177: LGTM!The example commands correctly reflect the updated device path, keeping documentation aligned with the code.
scripts/test_all_rpc.py (3)
7-7: LGTM!Device path update is consistent with other files in this PR.
70-86: LGTM with a note on the test approach.The test now:
- Passes a fixed
session_idtofrost_commit(rather than receiving one back)- Validates for
commitmentin the responseUsing a fixed session ID for testing is reasonable since this is a single-device test, and the same ID is correctly reused in
test_frost_sign. The validation change reflects the updated RPC contract where the device returns a commitment for the caller-provided session.
94-98: LGTM!The fixed
session_idmatches what was passed tofrost_commit, ensuring test consistency. Thecommit_resultparameter is still used to gate the test execution (line 90-92), which is appropriate.main/nostr_frost.h (2)
140-143: LGTM!The new DKG Round2 and NIP46 parsing APIs follow consistent naming conventions and integrate well with the existing FROST protocol functions.
Also applies to: 178-185
100-112: This comment is incorrect—there is no asymmetric memory management.
frost_create_nip46_responseis a serialization function that takes aconst nip46_response_t *responseand only reads theresultanderrorfields. It never allocates these fields. The library doesn't ownnip46_response_tinstances; callers create and own them. The absence offrost_nip46_response_freeis correct and intentional.By contrast,
frost_nip46_request_freeis needed becausenip46_request_tis parsed and allocated by the library, which dynamically allocatesrequest->params. The asymmetry you observed is actually the proper separation of memory ownership: types allocated by the library have free functions; types provided by callers do not.Likely an incorrect or invalid review comment.
main/main.c (1)
90-92: LGTM!The new
RPC_METHOD_GET_SHARE_INFOcase follows the established pattern of delegating to a frost_signer function, consistent with theGET_SHARE_PUBKEYhandler above it.main/protocol.c (1)
9-9: LGTM!The new method parsing entry for
"get_share_info"follows the established pattern and maintains consistency with the surrounding code.main/frost_signer.h (1)
10-10: LGTM!The new
frost_get_share_infofunction declaration follows the established pattern of other frost_signer functions, with consistent signature style and parameter naming.README.md (1)
176-176: LGTM!The documentation accurately describes the new
get_share_infoendpoint and aligns with the implementation that returns pubkey, index, threshold, and participants metadata.main/frost_coordinator.c (1)
134-144: LGTM! DKG Round2 and NIP46 event handling properly integrated.The new event handlers follow the established pattern and correctly manage resources:
- DKG Round2 parsing invokes the callback without additional cleanup (no dynamic allocations in
frost_dkg_round2_t)- NIP46 parsing appropriately frees the request after the callback (
nip46_request_t.paramsis heap-allocated)Both handlers verify callback existence before parsing, preventing unnecessary work.
main/nostr_frost.c (3)
991-1074: LGTM! DKG Round2 parsing follows established patterns.The function correctly:
- Validates event kind
- Extracts group_id, sender_index, and recipient_index from tags
- Decrypts content and parses the inner JSON payload
- Populates the
frost_dkg_round2_tstructure- Cleans up allocated resources
The strict decryption requirement (returning -3 on failure) is appropriate for DKG security.
1324-1376: LGTM! NIP46 request parsing correctly handles dynamic allocation.The function properly:
- Validates the NIP46 event kind
- Decrypts the content using the sender's pubkey
- Extracts id, method, and params from the decrypted JSON
- Allocates and stores params as a string (freed later by
frost_nip46_request_free)Resource management is correct:
decryptedis freed, and the caller is responsible for freeingrequest->params.
1434-1440: LGTM! Resource cleanup correctly frees NIP46 request params.The function safely frees the dynamically allocated
paramsfield and resets the pointer and length. It's idempotent and follows the same pattern asfrost_sign_request_free.
kwsantiago
force-pushed
the
feat/dkg-nip46-usb-fixes
branch
from
ddc49eb to
dbecfaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @main/nostr_frost.c:
- Around line 1351-1372: The code currently treats a failed
cJSON_Parse(decrypted) as success and leaves request partially initialized;
after calling cJSON_Parse and getting a NULL inner, free decrypted, clean up any
partially set fields on request (or zero them) and return an error status
instead of proceeding to the success path; specifically update the block around
cJSON_Parse/decrypted to check if inner == NULL, call free(decrypted),
cJSON_Delete if needed, ensure request->params (and params_len) and any other
fields are left in a predictable state, and propagate an error return from the
enclosing function so callers cannot use a partially populated request.
- Around line 1046-1049: frost_parse_dkg_round2_event currently rejects events
when decryption fails, but frost_create_dkg_round2_event silently falls back to
plaintext ("{}") on nip44_encrypt_content failure; make these symmetric by
changing frost_create_dkg_round2_event to treat nip44_encrypt_content failure as
a hard error (propagate/return an error code instead of inserting "{}"), remove
the plaintext fallback, and ensure error codes match the parser (use the same -3
or a consistent error) so Round2 events cannot be created as plaintext while the
parser requires decryption.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
README.mdmain/frost.cmain/frost.hmain/frost_coordinator.cmain/frost_coordinator.hmain/frost_signer.cmain/frost_signer.hmain/main.cmain/nostr_frost.cmain/nostr_frost.hmain/protocol.cmain/protocol.hscripts/monitor_serial.pyscripts/test_all_rpc.pyscripts/test_dkg_full.pyscripts/test_dkg_relay.pytest/hardware/test_hardware.pytest/integration/test_full_flow.sh
🚧 Files skipped from review as they are similar to previous changes (13)
- main/protocol.c
- README.md
- scripts/test_all_rpc.py
- main/frost_signer.c
- main/frost.c
- main/frost_coordinator.c
- test/integration/test_full_flow.sh
- test/hardware/test_hardware.py
- main/frost_signer.h
- scripts/test_dkg_relay.py
- main/frost.h
- scripts/monitor_serial.py
- main/frost_coordinator.h
🧰 Additional context used
🧬 Code graph analysis (2)
main/nostr_frost.h (1)
main/nostr_frost.c (3)
frost_parse_nip46_event(1324-1376)frost_create_nip46_response(1378-1434)frost_nip46_request_free(1436-1442)
main/main.c (1)
main/frost_signer.c (1)
frost_get_share_info(190-207)
⏰ 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 (8)
scripts/test_dkg_full.py (1)
15-15: LGTM! Device path updated correctly.The change from
/dev/ttyUSB0to/dev/ttyACM0reflects the correct USB device type for the hardware being tested (USB CDC ACM devices vs. USB-to-serial converters). This aligns with the PR's objective to fix USB device paths and is part of a broader standardization effort across test scripts.main/protocol.h (1)
27-27: LGTM!The new enum member is properly positioned and aligns with the related RPC dispatch logic added in main.c.
main/nostr_frost.h (2)
100-112: LGTM!The NIP-46 request and response structures are well-designed. The
nip46_request_tstruct has a corresponding cleanup function (frost_nip46_request_free), which is essential for the dynamically allocatedparamsfield.Note:
nip46_response_tpointers (result,error) are caller-owned and should be freed by the caller after use. Consider documenting this ownership contract in comments if not already documented elsewhere.
140-143: LGTM!The function prototypes follow existing conventions and provide complete coverage for DKG Round2 and NIP-46 operations, including proper cleanup functions.
Also applies to: 178-185
main/main.c (1)
90-92: LGTM!The new RPC method handler follows the established pattern and correctly delegates to
frost_get_share_info.main/nostr_frost.c (3)
1397-1408: LGTM!The error/result handling correctly addresses the concerns from past review comments:
- Mutual exclusivity is enforced via the
if-else-if-elsestructure- The fallback error (line 1407) ensures that a valid NIP-46 response is always created, even when both
errorandresultare NULLThe implementation aligns with NIP-46 semantics requiring either
errororresultin every response.
1410-1434: LGTM!The event creation, encryption, signing, and serialization flow is correctly implemented with proper error handling and resource cleanup at each step.
1436-1442: LGTM!The cleanup function follows best practices: null-checks the request pointer, frees dynamically allocated memory, and resets the pointer and length to prevent use-after-free.
kwsantiago
force-pushed
the
feat/dkg-nip46-usb-fixes
branch
from
dbecfaf to
0a40eff
Compare
1 participant
Summary by CodeRabbit
New Features
Configuration
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.