feat: multi-sig script (CIP-1854) #367
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a native-script and multisig subsystem: new Script and NativeScript type aliases, constructors for ScriptSig/All/Any/NofK and timelocks, CBOR JSON marshal/unmarshal helpers, Blake2b-based script hashing and address derivation, and script validation with per-type validators and timelock checks. Exposes script operations via CLI commands (create/validate/address) and HTTP endpoints (/api/script/create, /api/script/validate, /api/script/address) with OpenAPI models, generated client code, and docs. Adds CIP‑1854 multisig key-derivation helpers, KeyFile helpers, extensive tests, and many autogenerated OpenAPI model/doc updates. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 12 files
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="openapi.sh">
<violation number="1" location="openapi.sh:5">
P2: The `sed -i` command is not portable across macOS and Linux. On macOS (BSD sed), `-i` requires a backup suffix argument. Use `sed -i ''` for in-place editing without backup on macOS, or use a portable alternative.</violation>
</file>
<file name="bursa.go">
<violation number="1" location="bursa.go:1082">
P0: This function always returns `true`, completely bypassing signature validation. This is a critical security issue - any script validation will pass regardless of whether valid signatures are provided. If this is intentional placeholder code, it should not be merged to production.</violation>
</file>
<file name="internal/api/api.go">
<violation number="1" location="internal/api/api.go:1151">
P1: Direct interpolation of `err.Error()` into JSON string can cause JSON injection or malformed responses if the error contains quotes or backslashes. Either use `json.Marshal` for proper escaping, or return a generic error message like other handlers do for internal errors.</violation>
</file>
<file name="internal/cli/cli.go">
<violation number="1" location="internal/cli/cli.go:184">
P2: If both `timelockBefore` and `timelockAfter` are specified, `timelockAfter` is silently ignored. Consider either validating they aren't both set, or applying both timelocks to the script.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
openapi.sh
Outdated
| docker run --rm -v "${PWD}:/local" openapitools/openapi-generator-cli generate -i /local/docs/swagger.yaml --git-user-id blinklabs-io --git-repo-id bursa -g go -o /local/openapi -c /local/openapi-config.yml | ||
| make format golines | ||
| cd openapi && go mod tidy | ||
| cd openapi && sed -i 's/go 1.23/go 1.24.0/' go.mod && go mod tidy |
There was a problem hiding this comment.
P2: The sed -i command is not portable across macOS and Linux. On macOS (BSD sed), -i requires a backup suffix argument. Use sed -i '' for in-place editing without backup on macOS, or use a portable alternative.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At openapi.sh, line 5:
<comment>The `sed -i` command is not portable across macOS and Linux. On macOS (BSD sed), `-i` requires a backup suffix argument. Use `sed -i ''` for in-place editing without backup on macOS, or use a portable alternative.</comment>
<file context>
@@ -2,4 +2,4 @@
docker run --rm -v "${PWD}:/local" openapitools/openapi-generator-cli generate -i /local/docs/swagger.yaml --git-user-id blinklabs-io --git-repo-id bursa -g go -o /local/openapi -c /local/openapi-config.yml
make format golines
-cd openapi && go mod tidy
+cd openapi && sed -i 's/go 1.23/go 1.24.0/' go.mod && go mod tidy
</file context>
| cd openapi && sed -i 's/go 1.23/go 1.24.0/' go.mod && go mod tidy | |
| cd openapi && sed -i.bak 's/go 1.23/go 1.24.0/' go.mod && rm -f go.mod.bak && go mod tidy |
bursa.go
Outdated
| func validateScriptSig(script *ScriptSig, signatures [][]byte) bool { | ||
| // Placeholder: in a real implementation, this would verify signatures against the key hash | ||
| // For testing script logic, we assume the script is satisfied if signatures are provided | ||
| return true // Always return true for now to test script structure logic |
There was a problem hiding this comment.
P0: This function always returns true, completely bypassing signature validation. This is a critical security issue - any script validation will pass regardless of whether valid signatures are provided. If this is intentional placeholder code, it should not be merged to production.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bursa.go, line 1082:
<comment>This function always returns `true`, completely bypassing signature validation. This is a critical security issue - any script validation will pass regardless of whether valid signatures are provided. If this is intentional placeholder code, it should not be merged to production.</comment>
<file context>
@@ -724,6 +818,323 @@ func GetCommitteeHotExtendedSKey(committeeKey bip32.XPrv) (KeyFile, error) {
+func validateScriptSig(script *ScriptSig, signatures [][]byte) bool {
+ // Placeholder: in a real implementation, this would verify signatures against the key hash
+ // For testing script logic, we assume the script is satisfied if signatures are provided
+ return true // Always return true for now to test script structure logic
+}
+
</file context>
internal/api/api.go
Outdated
| logger.Error("failed to generate script address", "error", err) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| _, _ = fmt.Fprintf(w, `{"error":"%s"}`, err.Error()) |
There was a problem hiding this comment.
P1: Direct interpolation of err.Error() into JSON string can cause JSON injection or malformed responses if the error contains quotes or backslashes. Either use json.Marshal for proper escaping, or return a generic error message like other handlers do for internal errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/api/api.go, line 1151:
<comment>Direct interpolation of `err.Error()` into JSON string can cause JSON injection or malformed responses if the error contains quotes or backslashes. Either use `json.Marshal` for proper escaping, or return a generic error message like other handlers do for internal errors.</comment>
<file context>
@@ -835,3 +886,288 @@ func handleWalletUpdate(w http.ResponseWriter, r *http.Request) {
+ logger.Error("failed to generate script address", "error", err)
+ w.Header().Set("Content-Type", "application/json")
+ w.WriteHeader(http.StatusBadRequest)
+ _, _ = fmt.Fprintf(w, `{"error":"%s"}`, err.Error())
+ return
+ }
</file context>
| _, _ = fmt.Fprintf(w, `{"error":"%s"}`, err.Error()) | |
| _, _ = w.Write([]byte(`{"error":"Failed to generate script address"}`)) |
| } | ||
|
|
||
| // Apply timelock if specified | ||
| if timelockBefore > 0 { |
There was a problem hiding this comment.
P2: If both timelockBefore and timelockAfter are specified, timelockAfter is silently ignored. Consider either validating they aren't both set, or applying both timelocks to the script.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/cli/cli.go, line 184:
<comment>If both `timelockBefore` and `timelockAfter` are specified, `timelockAfter` is silently ignored. Consider either validating they aren't both set, or applying both timelocks to the script.</comment>
<file context>
@@ -119,3 +121,227 @@ func RunLoad(dir string, showSecrets bool) {
+ }
+
+ // Apply timelock if specified
+ if timelockBefore > 0 {
+ script = bursa.NewTimelockedScript(timelockBefore, true, script)
+ } else if timelockAfter > 0 {
</file context>
| if timelockBefore > 0 { | |
| if timelockBefore > 0 && timelockAfter > 0 { | |
| logger.Error("cannot specify both --timelock-before and --timelock-after") | |
| os.Exit(1) | |
| } | |
| if timelockBefore > 0 { |
wolf31o2
force-pushed
the
feat/cip-1854-multisig-support
branch
from
687f174 to
fd1c535
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openapi/api/openapi.yaml (1)
15-166: Missing OpenAPI documentation for new script endpoints.The script endpoints (
/api/script/create,/api/script/validate,/api/script/address) defined ininternal/api/api.goare not documented in the OpenAPI specification. Add the corresponding path definitions and request/response schemas to keep the spec in sync with the implementation.
♻️ Duplicate comments (3)
openapi.sh (1)
5-5: Cross-platformsed -icompatibility issue remains.The
sed -i ''syntax works on macOS but fails on GNU/Linux sed (which interprets''as a backup suffix). This script will break in Linux CI environments.Use the portable pattern suggested in the prior review:
-cd openapi && sed -i '' 's/go 1.23/go 1.24.0/' go.mod && go mod tidy +cd openapi && sed -i.bak 's/go 1.23/go 1.24.0/' go.mod && rm -f go.mod.bak && go mod tidybursa.go (1)
1078-1089: Signature validation still bypassed - always returnstrue.This function always returns
trueregardless of input: thelen(signatures) == 0branch returnstrue, and the else branch returnslen(signatures) > 0which is alwaystruewhen signatures exist.The TODO indicates this is intentional for testing, but this should not be merged to production without proper validation or clear documentation that this is a placeholder. At minimum, consider:
- Adding a prominent warning in the function docstring
- Using a build tag to disable in production
- Implementing actual signature verification against
script.KeyHash// validateScriptSig checks if a signature script is satisfied -func validateScriptSig(_ *ScriptSig, signatures [][]byte) bool { +// WARNING: This is a placeholder that always returns true. +// TODO: Implement Ed25519 signature verification against script.KeyHash +func validateScriptSig(script *ScriptSig, signatures [][]byte) bool { // Basic validation: for API/script structure validation, allow empty signatures - // For actual spending validation, signatures should be provided and verified - // TODO: Implement full Ed25519 signature verification against script.KeyHash - if len(signatures) == 0 { - // Allow empty signatures for basic script validation (API usage) - return true - } - // If signatures are provided, require at least one (basic check) - return len(signatures) > 0 + // SECURITY: This placeholder always returns true - not for production use + return true }internal/api/api.go (1)
1150-1163: Error handling correctly uses json.Marshal.The error response at lines 1156-1161 properly uses
json.Marshalto encode the error, preventing JSON injection. This addresses the concern from the past review comment.
🧹 Nitpick comments (8)
bursa.go (2)
946-960: Usingpanicfor input validation in library functions.
NewMultiSigScript,NewAllMultiSigScript, andNewAnyMultiSigScriptpanic on invalid input. This is unusual for a library API and prevents callers from handling errors gracefully.Consider returning errors instead of panicking, or document the panic behavior clearly:
-func NewMultiSigScript(required int, keyHashes ...[]byte) *ScriptNOf { - if required < 1 || required > len(keyHashes) { - panic("invalid required signatures count") - } - if len(keyHashes) == 0 { - panic("at least one key hash required") - } +func NewMultiSigScript(required int, keyHashes ...[]byte) (*ScriptNOf, error) { + if len(keyHashes) == 0 { + return nil, errors.New("at least one key hash required") + } + if required < 1 || required > len(keyHashes) { + return nil, fmt.Errorf("invalid required signatures count: %d (must be 1-%d)", required, len(keyHashes)) + }
1507-1561: Duplicate handling of pointer and value types inscriptToMap.The function duplicates logic for pointer types (
*ScriptSig, etc.) and value types (ScriptSig, etc.). This doubles the code and maintenance burden.Consider normalizing to pointer types or using a helper to reduce duplication:
func scriptToMap(script Script) (map[string]any, error) { // Normalize to pointer if needed switch s := script.(type) { case ScriptSig: return scriptToMap(&s) case ScriptAll: return scriptToMap(&s) // ... etc case *ScriptSig: return map[string]any{ "type": "sig", "keyHash": hex.EncodeToString(s.KeyHash), }, nil // ... only pointer cases below } }internal/api/api_test.go (1)
658-691: Tests cover happy path only; consider adding error case coverage.The test verifies successful script creation but doesn't cover error scenarios such as invalid JSON, missing required fields, invalid key hash format, or unsupported script types. Adding negative test cases would improve confidence in error handling.
Additionally, using
assert.Containson the raw body string is fragile—consider unmarshaling the JSON response and asserting on the struct fields directly.internal/api/api.go (3)
133-141: Validation forRequiredfield may allow invalid value of 0.The
ScriptCreateRequest.Requiredfield hasvalidate:"min=1"but the validation only applies when the field is present due toomitempty. WhenTypeis "nOf" andRequiredis 0 (default), validation passes but the handler correctly catches this at lines 960-967. Consider adding conditional validation or documenting this behavior.
973-982: Defensive default case has similar JSON injection risk.While validation should prevent reaching this code path, the
req.Typevalue is interpolated into JSON. For consistency with other error handling, consider usingjson.Marshal.
1070-1079: Validation always uses empty signatures and slot 0.The handler calls
ValidateScript(script, [][]byte{}, 0)with hardcoded empty signatures and slot 0. This provides basic structural validation but doesn't allow clients to test against specific conditions. Consider accepting optionalsignaturesandslotparameters in the request for more comprehensive validation.openapi/api/openapi.yaml (1)
253-317: Example values use inconsistent descriptions for different key types.All key file examples in
bursa.Walletuse "Payment Verification Key" as the description, even for stake, drep, and committee keys. While these are just example values, using type-appropriate descriptions would improve documentation clarity.Example for
stake_vkey:stake_vkey: description: Stake Verification Key type: StakeVerificationKeyShelley_ed25519 cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880openapi/model_bursa_wallet.go (1)
21-40: New BursaWallet fields and JSON tags look correct (with a small security consideration)The added Committee* and Drep* fields are all pointers to
BursaKeyFilewith snake_case JSON tags that match the docs and ToMap keys, keeping the model consistent and backwards-compatible. Since these represent additional governance-related signing keys, this also expands the surface of sensitive material that can be serialized; worth double-checking that API endpoints only expose these fields where explicitly intended and that logging/redaction policies treat them as secrets, in line with how existing payment/stake keys are handled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
bursa.go(4 hunks)bursa_test.go(2 hunks)cmd/bursa/main.go(1 hunks)cmd/bursa/script.go(1 hunks)internal/api/api.go(5 hunks)internal/api/api_test.go(1 hunks)internal/cli/cli.go(2 hunks)openapi.sh(1 hunks)openapi/api/openapi.yaml(5 hunks)openapi/api_default.go(12 hunks)openapi/docs/BursaWallet.md(2 hunks)openapi/model_bursa_wallet.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
cmd/bursa/script.go (1)
internal/cli/cli.go (4)
Run(31-33)RunScriptCreate(125-235)RunScriptValidate(237-301)RunScriptAddress(303-353)
internal/api/api.go (2)
bursa.go (11)
Script(68-73)NewMultiSigScript(946-960)NewAllMultiSigScript(963-974)NewAnyMultiSigScript(977-988)NewTimelockedScript(991-1002)MarshalScript(1414-1439)UnmarshalScript(1442-1447)ScriptData(1406-1411)ValidateScript(1059-1076)GetScriptHash(1025-1034)GetScriptAddress(1037-1056)internal/logging/logging.go (1)
GetLogger(78-83)
bursa_test.go (1)
bursa.go (24)
NewScriptSig(915-917)NewScriptAll(920-922)NewScriptAny(925-927)NewScriptNOf(930-932)NewScriptBefore(935-937)NewScriptAfter(940-942)GetScriptHash(1025-1034)GetScriptAddress(1037-1056)ErrInvalidNetwork(47-47)GetRootKeyFromMnemonic(490-504)GetMultiSigAccountKey(822-832)GetMultiSigPaymentKey(835-843)GetMultiSigPaymentVKey(846-863)GetMultiSigPaymentSKey(866-872)GetMultiSigStakeKey(875-883)GetMultiSigStakeVKey(886-903)GetMultiSigStakeSKey(906-912)ValidateScript(1059-1076)NewMultiSigScript(946-960)NewAllMultiSigScript(963-974)NewAnyMultiSigScript(977-988)NewTimelockedScript(991-1002)ScriptAll(90-92)NewMultiSigScriptFromKeys(1006-1022)
internal/cli/cli.go (2)
internal/logging/logging.go (1)
GetLogger(78-83)bursa.go (10)
Script(68-73)NewAllMultiSigScript(963-974)NewAnyMultiSigScript(977-988)NewMultiSigScript(946-960)NewTimelockedScript(991-1002)MarshalScript(1414-1439)ScriptData(1406-1411)UnmarshalScript(1442-1447)ValidateScript(1059-1076)GetScriptAddress(1037-1056)
openapi/model_bursa_wallet.go (2)
openapi/model_bursa_key_file.go (1)
BursaKeyFile(22-26)openapi/utils.go (1)
IsNil(335-352)
⏰ 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). (3)
- GitHub Check: Analyze (go)
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: docker
🔇 Additional comments (24)
openapi/api_default.go (1)
1-1040: Auto-generated file with cosmetic comment formatting changes.This is generated code with only whitespace adjustments in
@returndoc comments. No functional changes.bursa.go (1)
821-832: CIP-1854 multi-sig account key derivation looks correct.The derivation path
m/1854'/1815'/account'follows CIP-1854 specification for multi-signature wallets.cmd/bursa/main.go (1)
39-43: Clean integration of script command.The addition follows the existing pattern for other subcommands.
cmd/bursa/script.go (1)
22-168: Well-structured CLI commands for script operations.The Cobra command structure is clean with good documentation and examples. Flag definitions are appropriate, and required flags are properly marked. The delegation to
internal/clifunctions maintains good separation of concerns.internal/api/api_test.go (2)
693-727: LGTM with minor suggestion.The test exercises the validation endpoint correctly. The assertion
assert.Contains(t, string(body), "true")works but could be more precise by parsing the JSON and checkingresponse.Valid == true.
729-764: LGTM!The test correctly validates that the address endpoint returns a mainnet address (with
addr1prefix) for the given script and network.bursa_test.go (6)
882-948: Good coverage of script type constructors.Tests verify all six script types (Sig, All, Any, NOf, Before, After) with correct type identifiers and non-empty CBOR output. The 28-byte key hash used for ScriptSig aligns with Blake2b-224 output size.
950-967: LGTM!Tests correctly verify 28-byte script hash length and validate address generation for mainnet, including the error case for invalid networks.
969-998: LGTM!Thorough test of CIP-1854 multi-signature key derivation path, verifying the entire chain from mnemonic → root → account → payment/stake keys with correct key file types.
1000-1039: LGTM!Validation tests cover all script types with appropriate boundary conditions for timelocks. The comments clarify the design decision that ScriptSig allows empty signatures for basic structural validation.
1075-1089: LGTM!Tests correctly exercise script generation from Ed25519 public keys, verifying the complete flow from key generation to script validation.
1091-1103: LGTM!Edge case tests properly verify panic behavior for invalid multisig constructor parameters, ensuring the API contract is enforced.
internal/api/api.go (3)
143-153: LGTM!The request types use
map[string]interface{}for the Script field, which aligns with the flexible JSON structure expected bybursa.UnmarshalScript. Network validation correctly restricts to valid Cardano networks.
255-258: LGTM!Script routes are correctly registered and available regardless of GCP configuration.
532-537: LGTM!The error handling now uses
json.Marshalto safely encode error messages, preventing JSON injection issues from special characters in error strings.internal/cli/cli.go (4)
155-160: Timelock conflict validation correctly implemented.The code now validates that both
timelockBeforeandtimelockAftercannot be specified together, addressing the past review comment.
204-234: LGTM!File handling is properly implemented with deferred close and pretty-printed JSON output for better readability.
237-301: LGTM!The validation flow correctly reads the script file, parses signatures, validates, and outputs a structured JSON result. Using the hash from the script file maintains consistency.
303-353: LGTM!Address generation flow is consistent with other CLI commands, correctly reading the script file and outputting structured JSON.
openapi/api/openapi.yaml (1)
172-235: LGTM!Validation constraints are correctly specified and align with the Go struct validation tags. The maximum value of 2147483647 for ID fields correctly enforces the non-hardened derivation index limit.
openapi/docs/BursaWallet.md (2)
7-15: New governance key properties are documented consistentlyThe added Committee* and Drep* properties align in naming and types with
BursaWallet(allBursaKeyFile, all optional) and follow the existing pattern used for payment/stake keys, so the properties table looks correct and consistent.
45-268: Accessor method docs match the generated Go API surfaceThe
Get*,Get*Ok,Set*, andHas*entries for the new Committee* and Drep* fields mirror the actual methods onBursaWalletand follow the same wording and structure as the existing wallet accessors, so the docs accurately reflect the model.openapi/model_bursa_wallet.go (2)
60-345: Getter/Setter/Has helpers are correctly wired to their backing fieldsEach of the new
Get*,Get*Ok,Has*, andSet*methods for the Committee* and Drep* fields consistently references the matching struct field and follows the same OpenAPI-generator pattern used for the existing wallet fields, so there are no apparent copy/paste or wiring errors here.
644-672: ToMap serialization includes all new fields with matching JSON keysThe
ToMapimplementation conditionally adds each new Committee* and Drep* field when non-nil, using keys that match the struct tags (committee_cold_extended_skey,committee_cold_skey,committee_cold_vkey,committee_hot_*,drep_*). This keeps JSON output consistent with the schema and with the rest of the wallet fields.
wolf31o2
force-pushed
the
feat/cip-1854-multisig-support
branch
from
fd1c535 to
eeb1fd5
Compare
|
@cubic-dev-ai review this PR |
@wolf31o2 I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
6 issues found across 12 files
Prompt for AI agents (all 6 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="bursa.go">
<violation number="1" location="bursa.go:1095">
P2: `ValidateScript` only handles pointer types in its type switch, but the `Script` interface methods use value receivers. If a value type (e.g., `ScriptSig{}` instead of `*ScriptSig`) is passed, validation will silently fail by returning `false`. Consider adding value type cases like `scriptToMap` does, or document that only pointer types are supported.</violation>
<violation number="2" location="bursa.go:1654">
P2: Inconsistent validation: `NewMultiSigScript` requires `n >= 1`, but `mapToScript` allows `n >= 0`. An nOf script with n=0 would always be satisfied regardless of signatures. Consider changing the validation to `n < 1` to match `NewMultiSigScript`.</violation>
</file>
<file name="openapi.sh">
<violation number="1" location="openapi.sh:5">
P1: The `sed -i ''` syntax is macOS/BSD-specific and will fail on GNU/Linux systems. Consider using a portable approach that works across platforms.</violation>
</file>
<file name="internal/cli/cli.go">
<violation number="1" location="internal/cli/cli.go:234">
P2: When writing to a file, the error from `file.Close()` should be checked explicitly rather than deferred without error capture. If the close fails after encoding succeeds, data could be silently lost.</violation>
</file>
<file name="internal/api/api_test.go">
<violation number="1" location="internal/api/api_test.go:737">
P3: Misleading test case name: 'invalid JSON' should be 'invalid hex in keyHash' or 'invalid keyHash format'. The JSON is syntactically valid; the issue is the invalid hex string value.</violation>
</file>
<file name="internal/api/api.go">
<violation number="1" location="internal/api/api.go:1004">
P2: When both `TimelockBefore` and `TimelockAfter` are provided, `TimelockAfter` is silently ignored due to `else if`. Consider either applying both timelocks (which is valid in Cardano native scripts) or returning an error if both are provided to avoid silent unexpected behavior.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
bursa.go
Outdated
| if !ok { | ||
| return nil, errors.New("missing or invalid scripts for nOf script") | ||
| } | ||
| if n < 0 || n > len(scriptsInterface) { |
There was a problem hiding this comment.
P2: Inconsistent validation: NewMultiSigScript requires n >= 1, but mapToScript allows n >= 0. An nOf script with n=0 would always be satisfied regardless of signatures. Consider changing the validation to n < 1 to match NewMultiSigScript.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bursa.go, line 1654:
<comment>Inconsistent validation: `NewMultiSigScript` requires `n >= 1`, but `mapToScript` allows `n >= 0`. An nOf script with n=0 would always be satisfied regardless of signatures. Consider changing the validation to `n < 1` to match `NewMultiSigScript`.</comment>
<file context>
@@ -984,3 +1455,237 @@ func PrintLoadedKeys(keys []*LoadedKey, showSecrets bool) {
+ if !ok {
+ return nil, errors.New("missing or invalid scripts for nOf script")
+ }
+ if n < 0 || n > len(scriptsInterface) {
+ return nil, fmt.Errorf("invalid n value %d: must be between 0 and %d", n, len(scriptsInterface))
+ }
</file context>
| if n < 0 || n > len(scriptsInterface) { | |
| if n < 1 || n > len(scriptsInterface) { |
bursa.go
Outdated
| } | ||
|
|
||
| // ValidateScript checks if a script is satisfied given signatures and current slot | ||
| func ValidateScript(script Script, signatures [][]byte, slot uint64) bool { |
There was a problem hiding this comment.
P2: ValidateScript only handles pointer types in its type switch, but the Script interface methods use value receivers. If a value type (e.g., ScriptSig{} instead of *ScriptSig) is passed, validation will silently fail by returning false. Consider adding value type cases like scriptToMap does, or document that only pointer types are supported.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bursa.go, line 1095:
<comment>`ValidateScript` only handles pointer types in its type switch, but the `Script` interface methods use value receivers. If a value type (e.g., `ScriptSig{}` instead of `*ScriptSig`) is passed, validation will silently fail by returning `false`. Consider adding value type cases like `scriptToMap` does, or document that only pointer types are supported.</comment>
<file context>
@@ -724,6 +848,353 @@ func GetCommitteeHotExtendedSKey(committeeKey bip32.XPrv) (KeyFile, error) {
+}
+
+// ValidateScript checks if a script is satisfied given signatures and current slot
+func ValidateScript(script Script, signatures [][]byte, slot uint64) bool {
+ switch s := script.(type) {
+ case *ScriptSig:
</file context>
openapi.sh
Outdated
| docker run --rm -v "${PWD}:/local" openapitools/openapi-generator-cli generate -i /local/docs/swagger.yaml --git-user-id blinklabs-io --git-repo-id bursa -g go -o /local/openapi -c /local/openapi-config.yml | ||
| make format golines | ||
| cd openapi && go mod tidy | ||
| cd openapi && sed -i '' 's/go 1.23/go 1.24.0/' go.mod && go mod tidy |
There was a problem hiding this comment.
P1: The sed -i '' syntax is macOS/BSD-specific and will fail on GNU/Linux systems. Consider using a portable approach that works across platforms.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At openapi.sh, line 5:
<comment>The `sed -i ''` syntax is macOS/BSD-specific and will fail on GNU/Linux systems. Consider using a portable approach that works across platforms.</comment>
<file context>
@@ -2,4 +2,4 @@
docker run --rm -v "${PWD}:/local" openapitools/openapi-generator-cli generate -i /local/docs/swagger.yaml --git-user-id blinklabs-io --git-repo-id bursa -g go -o /local/openapi -c /local/openapi-config.yml
make format golines
-cd openapi && go mod tidy
+cd openapi && sed -i '' 's/go 1.23/go 1.24.0/' go.mod && go mod tidy
</file context>
| cd openapi && sed -i '' 's/go 1.23/go 1.24.0/' go.mod && go mod tidy | |
| cd openapi && sed -i.bak 's/go 1.23/go 1.24.0/' go.mod && rm -f go.mod.bak && go mod tidy |
internal/cli/cli.go
Outdated
| ) | ||
| os.Exit(1) | ||
| } | ||
| defer file.Close() |
There was a problem hiding this comment.
P2: When writing to a file, the error from file.Close() should be checked explicitly rather than deferred without error capture. If the close fails after encoding succeeds, data could be silently lost.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/cli/cli.go, line 234:
<comment>When writing to a file, the error from `file.Close()` should be checked explicitly rather than deferred without error capture. If the close fails after encoding succeeds, data could be silently lost.</comment>
<file context>
@@ -119,3 +121,250 @@ func RunLoad(dir string, showSecrets bool) {
+ )
+ os.Exit(1)
+ }
+ defer file.Close()
+
+ encoder := json.NewEncoder(file)
</file context>
internal/api/api_test.go
Outdated
| expectedError string | ||
| }{ | ||
| { | ||
| name: "invalid JSON", |
There was a problem hiding this comment.
P3: Misleading test case name: 'invalid JSON' should be 'invalid hex in keyHash' or 'invalid keyHash format'. The JSON is syntactically valid; the issue is the invalid hex string value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/api/api_test.go, line 737:
<comment>Misleading test case name: 'invalid JSON' should be 'invalid hex in keyHash' or 'invalid keyHash format'. The JSON is syntactically valid; the issue is the invalid hex string value.</comment>
<file context>
@@ -654,3 +654,197 @@ func TestWalletUpdateMethodGCPNotConfigured(t *testing.T) {
+ expectedError string
+ }{
+ {
+ name: "invalid JSON",
+ reqBody: `{
+ "script": {
</file context>
| name: "invalid JSON", | |
| name: "invalid keyHash format", |
| // Apply timelock if specified | ||
| if req.TimelockBefore > 0 { | ||
| script = bursa.NewTimelockedScript(req.TimelockBefore, true, script) | ||
| } else if req.TimelockAfter > 0 { |
There was a problem hiding this comment.
P2: When both TimelockBefore and TimelockAfter are provided, TimelockAfter is silently ignored due to else if. Consider either applying both timelocks (which is valid in Cardano native scripts) or returning an error if both are provided to avoid silent unexpected behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/api/api.go, line 1004:
<comment>When both `TimelockBefore` and `TimelockAfter` are provided, `TimelockAfter` is silently ignored due to `else if`. Consider either applying both timelocks (which is valid in Cardano native scripts) or returning an error if both are provided to avoid silent unexpected behavior.</comment>
<file context>
@@ -835,3 +893,339 @@ func handleWalletUpdate(w http.ResponseWriter, r *http.Request) {
+ // Apply timelock if specified
+ if req.TimelockBefore > 0 {
+ script = bursa.NewTimelockedScript(req.TimelockBefore, true, script)
+ } else if req.TimelockAfter > 0 {
+ script = bursa.NewTimelockedScript(req.TimelockAfter, false, script)
+ }
</file context>
eeb1fd5 to
85313f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/cli/cli.go (1)
220-323: CLIRunScriptValidatecurrently performs structural, not cryptographic, validation
RunScriptValidatereads aScriptDataJSON file, unmarshals it, parses signatures (if provided), and callsbursa.ValidateScript. BecauseValidateScripttreatslen(signatures) == 0as valid forScriptSigand composites, running:bursa script validate --script path.jsonwith no signatures will report
"valid": truefor any structurally well‑formed script.If you intend this CLI to support real spending validation later, consider:
- Adding an explicit
--structural-onlyflag, or- Requiring at least one signature for non‑timelock scripts and failing otherwise,
so callers can’t confuse structural checks with full signature verification.
bursa.go (1)
1094-1155:ValidateScript+validateScriptSigstill treat no signatures as valid – risky semanticsEven after the changes,
validateScriptSigreturnstruewhenlen(signatures) == 0, andValidateScriptdelegates to it forScriptSigand all composite scripts. This means:
- Any script containing only
sigconditions (including nestedall,any,nOf) validates astruewhen called with an empty signature list.- The API (
handleScriptValidate) and CLI (RunScriptValidate) both callValidateScriptexactly this way for “structure-only” validation and assert thatvalid == truewith no signatures.That behavior is explicitly documented as “structural validation only”, but the public function name
ValidateScriptand its exported status make it easy for callers to misuse it as a full signature validator, accidentally bypassing real cryptographic checks.Consider one of these hardening options:
- Split the concerns: introduce a separate
ValidateScriptStructure(no signatures) and reserveValidateScriptfor cryptographic validation that always requires non‑empty signatures and eventually verifies Ed25519 signatures againstScriptSig.KeyHash.- Or add an explicit
mode(e.g.validateSignatures bool) parameter so production paths cannot accidentally rely on the structural‑only behavior.Once you introduce real signature verification, you’ll also need to adjust the tests in
TestValidateScriptand the script‑validation API / CLI to distinguish between structural checks and spending validation.internal/api/api.go (1)
1041-1143:handleScriptValidateexposes structural validity only (no signature requirement)
handleScriptValidate:
- Validates the request payload,
- Unmarshals the script from the provided JSON,
- Optionally decodes signatures from hex, and
- Calls
bursa.ValidateScript(script, signatures, req.Slot).Because
ValidateScriptcurrently treatslen(signatures) == 0as valid forScriptSigand composites, the endpoint will return"valid": truefor any structurally correct script even when"signatures"is omitted or empty.This is fine for a “does the script structure make sense?” endpoint, but the name and shape (
ScriptValidateRequest/Response) suggest full validation. Consider either:
- Explicitly documenting that this is a structural validator only, and/or
- Introducing a stricter mode or a separate endpoint once real Ed25519 verification is added, so clients can’t accidentally treat this as authoritative spending validation.
🧹 Nitpick comments (6)
openapi/api/openapi.yaml (1)
236-359: Wallet/KeyFile examples & secret key fields – verify intended exposureThe expanded
bursa.KeyFileandbursa.Walletschemas (with examples for mnemonic, payment/stake addresses, and multiple *skey/*vkey fields, including DRep and committee keys) look structurally correct and match the generatedBursaWallet/BursaKeyFilemodels. Just ensure:
- These secret-bearing fields are indeed meant to be part of the documented HTTP response surface, and
- Example
descriptionstrings (“Payment Verification Key”) reused across all key types are acceptable, or update them later for better clarity.Consider explicitly reviewing which of these fields should be returned from public APIs (vs. only in secure/storage contexts) and, if needed, documenting that in the OpenAPI descriptions.
openapi/model_bursa_wallet.go (1)
21-41: Generated wallet accessors and serialization look consistentThe added committee/DRep key fields and their
Get*/Has*/Set*helpers follow the same pattern as existing wallet fields, and theToMapupdates correctly include them only when non-nil. Functionally this is fine; just be aware this expands the JSON surface for key material wheneverBursaWalletis serialized via the OpenAPI layer.If you later want a “public view” of wallets without secret keys, consider introducing a separate DTO type instead of overloading
BursaWalletfor all contexts.Also applies to: 60-347, 644-672
bursa.go (2)
67-188: Native script types & CBOR encoding are structurally soundThe Script interface and concrete types (
ScriptSig,ScriptAll,ScriptAny,ScriptNOf,ScriptBefore,ScriptAfter) match the expected native script layout ([tag, payload...]), and CBOR encoding now properly propagates errors. The encode–decode round-trip inScriptAll/Any/NOf.CBOR()is a bit inefficient but guarantees correctly nested CBOR structures.If CBOR performance ever becomes an issue, you could avoid the encode+decode dance by directly building nested
[]anystructures without intermediate encoding, but it’s not a blocker for typical script sizes.
1471-1703: Script (de)serialization helpers are coherent and recursion‑safe
ScriptData,MarshalScript,UnmarshalScript,scriptToMap, andmapToScriptform a consistent round‑trip:
scriptToMapnormalizes value vs pointer receivers and recursively converts nested scripts into JSON‑friendly maps.MarshalScriptoptionally derives an address when a network is provided and always includes a Blake2b‑224 hash.UnmarshalScript+mapToScriptcorrectly reconstruct nested scripts, with clear error messages for malformed inputs.The hard‑coded
"NativeScript"type discriminator is fine for now; if you later add Plutus or other script types, consider making the discriminator an enum and extending the switch inUnmarshalScriptto future‑proof the API.bursa_test.go (1)
882-1125: Script tests provide good coverage but encode placeholder validation semanticsThe new tests thoroughly exercise:
- Script construction and CBOR (
TestScriptTypes,TestMultiSigScriptGeneration),- Hashing and address derivation (
TestGetScriptHash,TestGetScriptAddress),- CIP‑1854 multi‑sig derivation helpers (
TestMultiSigKeyDerivation),- JSON round‑trips via
NewMultiSigScriptFromKeysand the script helpers.They also assert that
ValidateScript(..., nil, slot)returnstrueforScriptSig,all,any, andnOfvariants, and that a 2‑of‑3 script from keys validates with no signatures (TestValidateScript,TestMultiSigScriptFromKeys). This matches the current “structural only” behavior but will need to change once real signature checking is implemented.When you add Ed25519 verification, you may want to split these into (a) structure/timelock tests that don’t use signatures and (b) separate tests that supply actual signatures, so tightening validation doesn’t require large test rewrites.
internal/api/api.go (1)
133-141: Key hash and signature length constraints – double‑check against specCurrent tags enforce:
KeyHashes []string 'validate:"required,min=1,dive,hexadecimal,len=64"'→ each key hash must be 64 hex chars (32 bytes).Signatures []string 'validate:"dive,hexadecimal,len=128"'→ each signature must be 128 hex chars (64 bytes), which matches Ed25519.The signature constraint looks correct, but the 32‑byte key hash length may not match Cardano’s usual 28‑byte
ed25519KeyHash(56 hex chars) used in native scripts.Please confirm whether your “key_hashes” are intended to be raw public keys or 28‑byte key hashes per the ledger spec. If the latter, adjust
len=64tolen=56(and update tests and examples) to avoid generating scripts that won’t be accepted by downstream tooling.Also applies to: 145-148
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
bursa.go(4 hunks)bursa_test.go(2 hunks)cmd/bursa/main.go(1 hunks)cmd/bursa/script.go(1 hunks)internal/api/api.go(5 hunks)internal/api/api_test.go(1 hunks)internal/cli/cli.go(2 hunks)openapi.sh(1 hunks)openapi/api/openapi.yaml(5 hunks)openapi/api_default.go(12 hunks)openapi/docs/BursaWallet.md(2 hunks)openapi/model_bursa_wallet.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- openapi/api_default.go
- openapi.sh
- cmd/bursa/script.go
🧰 Additional context used
🧬 Code graph analysis (3)
bursa_test.go (1)
bursa.go (16)
NewScriptSig(945-947)NewScriptAll(950-952)NewScriptAny(955-957)NewScriptNOf(960-962)NewScriptBefore(965-967)NewScriptAfter(970-972)GetScriptHash(1055-1067)GetScriptAddress(1070-1092)ErrInvalidNetwork(47-47)ValidateScript(1095-1124)NewMultiSigScript(976-990)NewAllMultiSigScript(993-1004)NewAnyMultiSigScript(1007-1018)NewTimelockedScript(1021-1032)ScriptAll(89-91)NewMultiSigScriptFromKeys(1036-1052)
bursa.go (1)
bip32/bip32.go (2)
XPrv(43-43)PublicKey(49-49)
openapi/model_bursa_wallet.go (2)
openapi/model_bursa_key_file.go (1)
BursaKeyFile(22-26)openapi/utils.go (1)
IsNil(335-352)
⏰ 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). (3)
- GitHub Check: docker
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
cmd/bursa/main.go (1)
39-43: CLI wiring forscriptsubcommand looks correctAdding
scriptCommand()alongsidewalletCommand()andapiCommand()cleanly exposes the new script CLI surface from the root command; no issues spotted here.openapi/api/openapi.yaml (1)
169-235: Request validation constraints align with server-side validationThe added
maxLength/minLengthandmaximumconstraints for wallet delete/get/restore/update requests mirror the Govalidatortags (min=1,max=100,max=500,max=2147483647) ininternal/api/api.go, so the OpenAPI spec is consistent with backend validation behavior.If you ever change the Go-side
validatetags, remember to update these schema constraints in lockstep to avoid client/server drift. You can confirm current tags by greppingvalidate:"ininternal/api/api.go.bursa.go (1)
851-942: CIP‑1854 multi‑sig derivation helpers look correct
GetMultiSigAccountKey,GetMultiSigPaymentKey, andGetMultiSigStakeKeymirror the existing CIP‑1852 paths but switch purposefully to the 1854’ purpose index, and they enforce< 0x80000000bounds consistently withErrInvalidDerivationIndex. Keyfile constructors for multi‑sig payment/stake keys reuse the existing CBOR/keyfile patterns with adjusted descriptions, which is appropriate.If you’re targeting a specific CIP‑1854 revision, it’s worth double‑checking the exact derivation path (
1854' / 1815' / account' / role / index) against the current spec to ensure long‑term interoperability.internal/cli/cli.go (2)
17-28:RunScriptCreateflag validation and script construction are robustThe new
RunScriptCreatefunction:
- Enforces mutual exclusivity between
--all,--any, and--required, and validatesrequired <= len(keyHashes).- Ensures at least one key hash, and that
--timelock-beforeand--timelock-afteraren’t both set.- Parses key hashes safely from hex and delegates to the appropriate
bursa.New*MultiSigScripthelper.- Applies timelocks via
NewTimelockedScriptand marshals withbursa.MarshalScript.- Correctly checks
file.Close()errors in the deferred close.This wiring is consistent with the API behavior and handles user mistakes gracefully.
Also applies to: 125-218
325-375:RunScriptAddresscleanly reuses script JSON for address derivation
RunScriptAddressmirrors the validation handler: it loads aScriptDataJSON file, unmarshals viabursa.UnmarshalScript, derives an address withbursa.GetScriptAddress, and emits a small JSON envelope containingaddress,network, andscriptHash. Error handling and Content‑Type usage are consistent with the rest of the CLI.internal/api/api_test.go (1)
658-850: Script API tests cover core success and error paths wellThe new tests:
- Verify
/api/script/createreturns a JSON body withtype,script,address, andhash.- Exercise
/api/script/validatefor a happy path and multiple failure modes (bad keyHash hex, missingn/scripts, invalid network, unsupported script type).- Confirm
/api/script/addressresponds withaddress,network,scriptHash, and anaddr1-prefixed address.These align with the handlers’ behavior and should catch most regressions around script JSON shape and validation.
internal/api/api.go (3)
515-547: Wallet restore error responses now JSON‑encode messages safelyThe updated error handling in
handleWalletRestorefor client‑side errors:errorResp := map[string]string{"error": err.Error()} if jsonBytes, jsonErr := json.Marshal(errorResp); jsonErr == nil { _, _ = w.Write(jsonBytes) } else { _, _ = w.Write([]byte(`{"error":"Invalid request parameters"}`)) }avoids interpolating
err.Error()directly into a JSON string and uses a safe fallback. This removes the earlier JSON‑injection risk and keeps responses consistent with the rest of the API.
257-261: Script routes andhandleScriptCreatehandler behavior look consistent with CLIThe new
/api/script/createroute andhandleScriptCreatehandler:
- Decode
ScriptCreateRequest, validate fields (includingtype,key_hashes, and network), and log via the existing logger.- Decode key hashes from hex with clear errors per element and safe JSON encoding in error responses.
- Dispatch correctly to
bursa.NewMultiSigScript,NewAllMultiSigScript, orNewAnyMultiSigScriptwith an extra server-side check thatrequiredis within[1, len(hashes)]fornOf.- Reject configurations where both
timelock_beforeandtimelock_afterare set, mirroring the CLI.- Marshal the final script via
bursa.MarshalScriptand return its JSON (type/script/address/hash) with proper content type.Aside from the
Requiredtag issue noted earlier, this is a solid and defensive implementation.Also applies to: 897-1039
1145-1241:handleScriptAddresscorrectly reuses script JSON and handles errors safelyThe address endpoint:
- Validates
ScriptAddressRequest, includingnetworkbeing one ofmainnet/testnet.- Reuses the same
UnmarshalScriptpath as validation, failing fast on malformed script maps.- Calls
bursa.GetScriptAddressand, on error, returns a JSON‑encoded{"error": "..."}
usingjson.Marshalto avoid injection.- Computes the script hash via
GetScriptHashand returns a conciseScriptAddressResponse.This handler is well‑structured and consistent with the rest of the API error‑handling pattern.
openapi/docs/BursaWallet.md (1)
7-15: Verify documentation matches the generated Go model inopenapi/model_bursa_wallet.go.This documentation file lists 9 new wallet key fields (CommitteeCold*, CommitteeHot*, Drep*) and their corresponding accessor methods, which are consistent in structure and well-documented. However, since this appears to be OpenAPI documentation (typically auto-generated), ensure the documentation is in sync with the actual Go struct and its methods.
Please verify one of the following:
- If this documentation is auto-generated from the Go code: confirm it was regenerated as part of this PR using the OpenAPI generator.
- If this documentation is hand-maintained: confirm that all 9 fields and their 36 accessor methods (4 methods × 9 fields) are accurately reflected in
openapi/model_bursa_wallet.go.You can verify this by checking that:
- All field names match (CommitteeColdExtendedSkey, CommitteeColdSkey, etc.)
- All method signatures match (Get, GetOk, Set, Has for each field)
- No methods are missing or documented incorrectly
Also applies to: 45-268
wolf31o2
force-pushed
the
feat/cip-1854-multisig-support
branch
from
85313f0 to
767c925
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
openapi.sh (1)
5-5: Anchor thesedreplacement to avoid accidental partial matches.
's/go 1.23/go 1.24.0/'will also rewrite strings likego 1.23.1togo 1.24.0.1. Consider anchoring the line instead:cd openapi && if [[ "$OSTYPE" == "darwin"* ]]; then sed -i '' 's/^go 1\.23$/go 1.24.0/' go.mod else sed -i 's/^go 1\.23$/go 1.24.0/' go.mod fi && go mod tidyopenapi/api/openapi.yaml (1)
169-359: Derivation constraints now align with wallet logic; consider key example wording.The new
maxLength/minLengthandmaximum: 2147483647constraints match the derivation guardrails enforced inNewWallet/Get*Key (indices< 2^31), which is good. The sharedbursa.KeyFileexamples still say “Payment Verification Key” even when referenced for DRep/committee keys; if you care about docs clarity, you might tailor examples per field, but it’s not required for correctness.cmd/bursa/script.go (1)
36-96: Clarify key-hash examples inscript createhelp.The examples use placeholders like
key1.hash, but the flag description says key hashes must be hex-encoded. To avoid confusion, consider showing realistic hex snippets (e.g.abcdef...) or explicitly labeling them as hex hashes.bursa_test.go (1)
882-1125: Extend script tests to covervalidateSignatures=trueand numeric edge cases.The new tests do a nice job of exercising structural behavior (
ValidateScript(..., false)), script construction, and edge cases on required counts. To harden this further:
- Add tests that call
ValidateScriptwithvalidateSignatures=true, including invalid signature lengths and insufficient counts forNOf/Allscripts.- Add boundary tests for timelocks (slot exactly equal to the configured
before/afterslot).- Consider tests that round-trip
Script⇄ScriptDataviaMarshalScript/UnmarshalScriptincluding badn/slotinputs.These will make it easier to safely evolve the placeholder validation logic in
bursa.go.internal/cli/cli.go (1)
259-330: Reconsider unconditional “signatures required” gate inRunScriptValidate.Current behavior:
if !structuralOnly && len(sigBytes) == 0 { logger.Error("signatures required for cryptographic validation ...") os.Exit(1) } valid := bursa.ValidateScript(script, sigBytes, slot, !structuralOnly)This forbids using “cryptographic” mode for scripts that contain only timelocks and no
sigclauses, even thoughValidateScriptitself doesn’t require signatures forbefore/afterscripts.Options:
- Drop this gate and rely on
ValidateScriptto returnfalsewhere signatures truly are required, or- Make the gate conditional on the script actually containing signature clauses (e.g., a small helper that recursively checks for
signodes).This would bring CLI behavior in line with the help text and with timelock-only use cases.
internal/api/api_test.go (1)
658-850: Script handler tests look good; consider tightening the success assertion.The new tests cover create/validate/address handlers and common error cases, which is helpful. In
TestHandleScriptValidate, you currently assert that the body contains"true", which could match unrelated fields. If you want stronger guarantees, you might decode the JSON and assertvalid == trueexplicitly.Otherwise the structure and coverage of these tests look solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
bursa.go(4 hunks)bursa_test.go(2 hunks)cmd/bursa/main.go(1 hunks)cmd/bursa/script.go(1 hunks)internal/api/api.go(5 hunks)internal/api/api_test.go(1 hunks)internal/cli/cli.go(2 hunks)openapi.sh(1 hunks)openapi/api/openapi.yaml(5 hunks)openapi/api_default.go(12 hunks)openapi/docs/BursaWallet.md(2 hunks)openapi/model_bursa_wallet.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- openapi/api_default.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/bursa/main.go
🧰 Additional context used
🧬 Code graph analysis (5)
cmd/bursa/script.go (1)
internal/cli/cli.go (4)
Run(31-33)RunScriptCreate(125-257)RunScriptValidate(259-330)RunScriptAddress(332-382)
bursa_test.go (1)
bursa.go (24)
NewScriptSig(945-947)NewScriptAll(950-952)NewScriptAny(955-957)NewScriptNOf(960-962)NewScriptBefore(965-967)NewScriptAfter(970-972)GetScriptHash(1055-1067)GetScriptAddress(1070-1092)ErrInvalidNetwork(47-47)GetRootKeyFromMnemonic(520-534)GetMultiSigAccountKey(852-862)GetMultiSigPaymentKey(865-873)GetMultiSigPaymentVKey(876-893)GetMultiSigPaymentSKey(896-902)GetMultiSigStakeKey(905-913)GetMultiSigStakeVKey(916-933)GetMultiSigStakeSKey(936-942)ValidateScript(1097-1126)NewMultiSigScript(976-990)NewAllMultiSigScript(993-1004)NewAnyMultiSigScript(1007-1018)NewTimelockedScript(1021-1032)ScriptAll(89-91)NewMultiSigScriptFromKeys(1036-1052)
openapi/model_bursa_wallet.go (2)
openapi/model_bursa_key_file.go (1)
BursaKeyFile(22-26)openapi/utils.go (1)
IsNil(335-352)
bursa.go (1)
bip32/bip32.go (2)
XPrv(43-43)PublicKey(49-49)
internal/api/api.go (2)
bursa.go (11)
Script(68-73)NewMultiSigScript(976-990)NewAllMultiSigScript(993-1004)NewAnyMultiSigScript(1007-1018)NewTimelockedScript(1021-1032)MarshalScript(1491-1519)UnmarshalScript(1522-1527)ScriptData(1483-1488)ValidateScript(1097-1126)GetScriptHash(1055-1067)GetScriptAddress(1070-1092)internal/logging/logging.go (1)
GetLogger(78-83)
⏰ 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). (4)
- GitHub Check: Analyze (go)
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: docker
- GitHub Check: nilaway
🔇 Additional comments (7)
openapi/model_bursa_wallet.go (1)
21-41: New committee/DRep wallet fields and accessors look consistent.Struct additions, getters/setters,
Has*helpers, andToMapwiring for the committee/DRep key fields follow the existing pattern and should interoperate cleanly with the OpenAPI schema.Also applies to: 60-347, 644-672
internal/api/api.go (6)
258-261: LGTM!The script route registration is clean and follows the established pattern for API endpoints.
535-540: Good fix for JSON injection concern.The use of
json.Marshalproperly escapes error messages, addressing the JSON injection risk from previous reviews. The fallback to a generic message if marshaling fails is also appropriate.
942-958: Safe hex decoding with proper error handling.The hex decoding logic correctly uses
json.Marshalto safely construct error responses, preventing JSON injection.
1003-1018: Excellent fix for timelock mutual exclusivity.The code now explicitly validates that both
TimelockBeforeandTimelockAfterare not specified simultaneously, returning a clear error instead of silently ignoring one. This addresses the concern raised in previous reviews.
1042-1144: LGTM!The
handleScriptValidateimplementation follows best practices:
- Proper request validation
- Safe error handling with
json.Marshal- Individual signature format validation
- Clean response construction
1203-1216: Safe error handling - past issue resolved.The address generation error handling now uses
json.Marshalto safely construct error responses, addressing the JSON injection concern flagged in previous reviews. The fallback to a generic message is appropriate.
wolf31o2
force-pushed
the
feat/cip-1854-multisig-support
branch
from
767c925 to
33921aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
bursa.go(4 hunks)bursa_test.go(2 hunks)cmd/bursa/main.go(1 hunks)cmd/bursa/script.go(1 hunks)internal/api/api.go(5 hunks)internal/api/api_test.go(1 hunks)internal/cli/cli.go(2 hunks)openapi.sh(1 hunks)openapi/api/openapi.yaml(5 hunks)openapi/api_default.go(12 hunks)openapi/docs/BursaWallet.md(2 hunks)openapi/model_bursa_wallet.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- openapi/api_default.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/api/api_test.go
- openapi.sh
🧰 Additional context used
🧬 Code graph analysis (5)
cmd/bursa/script.go (1)
internal/cli/cli.go (4)
Run(31-33)RunScriptCreate(125-257)RunScriptValidate(287-374)RunScriptAddress(376-434)
internal/cli/cli.go (2)
internal/logging/logging.go (1)
GetLogger(78-83)bursa.go (16)
Script(68-73)NewAllMultiSigScript(997-1008)NewAnyMultiSigScript(1011-1022)NewMultiSigScript(976-994)MarshalScript(1510-1538)ScriptSig(76-78)ScriptAll(89-91)ScriptAny(114-116)ScriptNOf(139-142)ScriptBefore(165-167)ScriptAfter(178-180)ScriptData(1502-1507)UnmarshalScript(1541-1546)GetScriptHash(1059-1071)ValidateScript(1101-1135)GetScriptAddress(1074-1096)
openapi/model_bursa_wallet.go (2)
openapi/model_bursa_key_file.go (1)
BursaKeyFile(22-26)openapi/utils.go (1)
IsNil(335-352)
internal/api/api.go (2)
bursa.go (11)
Script(68-73)NewMultiSigScript(976-994)NewAllMultiSigScript(997-1008)NewAnyMultiSigScript(1011-1022)NewTimelockedScript(1025-1036)MarshalScript(1510-1538)UnmarshalScript(1541-1546)ScriptData(1502-1507)ValidateScript(1101-1135)GetScriptHash(1059-1071)GetScriptAddress(1074-1096)internal/logging/logging.go (1)
GetLogger(78-83)
bursa.go (1)
bip32/bip32.go (2)
XPrv(43-43)PublicKey(49-49)
⏰ 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). (4)
- GitHub Check: docker
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
🔇 Additional comments (35)
cmd/bursa/main.go (1)
42-42: LGTM!Clean integration of the new
scriptCommand()into the root command structure.cmd/bursa/script.go (4)
22-34: LGTM!Clean parent command setup for the script subcommand group.
36-97: LGTM!Well-structured command with clear help text examples covering all script creation modes (N-of-M, all, any, timelocked).
99-143: LGTM!Help text examples now correctly demonstrate the
--structural-onlyflag for timelock-only validation, addressing the previous review concern. ThepaniconMarkFlagRequiredfailure is acceptable as this only occurs during program initialization for programming errors.
145-177: LGTM!Clean implementation with sensible defaults and required flag enforcement.
bursa_test.go (7)
882-954: LGTM!Comprehensive test coverage for all script types (Sig, All, Any, NOf, Before, After) with proper type assertions and CBOR encoding verification.
956-974: LGTM!Good coverage of script hash length verification and address generation with both valid and invalid network scenarios.
976-1005: LGTM!Thorough test of the CIP-1854 multi-sig key derivation hierarchy, validating key types at each level.
1007-1058: LGTM!Good coverage of script validation across all script types, including timelock boundary conditions.
1060-1137: LGTM!Comprehensive testing of multi-sig script construction including N-of-M, All, Any variants, timelocked wrappers, and edge case error handling.
1139-1284: LGTM!Thorough testing of signature validation requirements and timelock boundary conditions with precise edge case coverage.
1286-1361: LGTM!Good round-trip testing for script serialization with proper error handling validation for invalid script types and malformed data.
internal/cli/cli.go (5)
18-19: LGTM!Appropriate imports added for hex encoding and JSON marshaling.
125-257: LGTM!Well-structured implementation with comprehensive validation addressing all previous review concerns:
- Mutual exclusivity of
--timelock-beforeand--timelock-after(Lines 155-160)- Validation that
requireddoesn't exceed key hash count (Lines 179-189)- Proper error handling on file close (Lines 234-239)
259-285: LGTM!Clean recursive implementation to determine if a script tree requires signatures.
287-374: LGTM!Good implementation that recomputes the script hash from the parsed script rather than trusting the file contents, addressing the previous review concern about hash integrity.
376-434: LGTM!Clean implementation with proper script hash recomputation from the parsed script.
internal/api/api.go (7)
19-19: LGTM!Required import for hex encoding of script hashes.
133-179: LGTM!Well-structured request/response types with correct validation:
Requiredfield validation fixed withomitemptyto allow zero value forall/anytypes- Key hash length (56 hex chars = 28 bytes) matches Blake2b-224 output
- Signature length (128 hex chars = 64 bytes) matches Ed25519 signature size
258-261: LGTM!Clean route registration for the new script endpoints.
535-540: LGTM!Safe error response handling using
json.Marshalto prevent JSON injection.
909-1046: LGTM!Well-implemented handler with:
- Safe error responses using
json.Marshal- Explicit mutual exclusion check for
timelock_beforeandtimelock_after(Lines 1007-1019)- Proper validation and error handling throughout
1059-1161: LGTM!Clean implementation with proper script hash computation from the parsed script.
1174-1260: LGTM!Safe implementation with
json.Marshalfor error responses, addressing the previous JSON injection concern.openapi/docs/BursaWallet.md (1)
1-497: Auto-generated documentation accurately reflects model changes.The documentation correctly describes the new committee and DRep key accessors following the established Get/GetOk/Set/Has pattern.
openapi/model_bursa_wallet.go (3)
22-41: Auto-generated struct fields follow established patterns.The new committee and DRep key fields correctly use pointer types for optional values and follow the existing naming and tagging conventions.
60-346: Generated accessor methods are consistent and correct.All 36 new accessor methods (Get/GetOk/Has/Set for 9 fields) follow the established pattern and use IsNil checks appropriately.
644-701: ToMap serialization correctly handles new fields.The serialization logic properly uses IsNil checks to conditionally include committee and DRep fields, maintaining consistency with existing field handling.
openapi/api/openapi.yaml (2)
172-230: Validation constraints align with implementation limits.The maximum value of 2147483647 (2^31-1) for ID fields correctly matches the BIP32 hardened derivation constraint enforced in the code. Name and description length limits are reasonable.
319-358: Schema structure correctly extends wallet model.The new committee and DRep key properties follow the established pattern of referencing bursa.KeyFile, maintaining consistency with existing payment and stake key fields.
bursa.go (5)
67-189: Script types correctly implement CBOR encoding with error handling.The CBOR methods now properly return errors, addressing previous review feedback. The decode/re-encode pattern in composite scripts (lines 98-110, 122-134, 148-160) ensures correct CBOR array structure for nested scripts.
851-942: Multi-sig key derivation correctly implements CIP-1854.The derivation path m/1854'/1815'/account' (line 858-860) properly uses purpose 1854' for multi-signature wallets as specified in CIP-1854, distinguishing them from standard single-key wallets that use 1852'.
974-994: Multi-sig constructor properly validates threshold.Line 980 correctly enforces
required >= 1, preventing the creation of 0-of-N scripts that would always be satisfied regardless of signatures (addressing previous review feedback).
1058-1096: Script hashing and addressing implementation is correct.Blake2b-224 (line 1064) is the correct hash function for Cardano script hashes. The panic at line 1067 is acceptable since blake2b.New only fails with invalid parameters. Script addresses correctly use AddressTypeScriptNone with no stake component.
1686-1750: JSON deserialization properly validates numeric constraints.The validation logic correctly addresses previous review feedback:
- Lines 1691-1693: Rejects fractional
nvalues for nOf scripts- Line 1699: Enforces
n >= 1(not>= 0), matching constructor validation- Lines 1728-1732, 1740-1744: Rejects negative and fractional slot values for timelocks
This prevents malformed JSON from creating unexpected script semantics.
wolf31o2
force-pushed
the
feat/cip-1854-multisig-support
branch
from
33921aa to
3c87157
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
openapi.sh (1)
5-5: Portability issue resolved with OS detection.The conditional check for
$OSTYPEcorrectly handles thesed -isyntax differences between macOS (BSD) and Linux (GNU). This addresses the previously flagged portability concerns.internal/cli/cli.go (4)
125-160: Past issues resolved: timelocks and required validation.The validation logic correctly handles:
- Mutual exclusivity of
--timelock-beforeand--timelock-after(lines 155-160)- Mutual exclusivity of
--all,--any, and--requiredflags- Minimum key hash requirement
179-189: Past issue resolved: required count validation.The validation that
requiredcannot exceed the number of key hashes has been correctly implemented.
234-239: Past issue resolved: file close error handling.The deferred file close now properly checks and handles errors, addressing the concern about silent data loss.
322-328: Past issue resolved: script hash recomputation.The script hash is now correctly recomputed from the unmarshaled script rather than trusting the file contents, addressing the previous concern about hash integrity.
internal/api/api.go (4)
133-141: Past issue resolved: validation tag fix.The
Requiredfield validation tag now correctly usesrequired_if=Type nOf,omitempty,min=1, which allows zero values forall/anytypes while enforcing validation whenType=="nOf".
535-540: Past issue resolved: JSON injection prevention.Error responses now use proper JSON marshaling via
json.Marshal(errorResp)with a safe fallback, preventing potential JSON injection from error messages containing special characters.
1006-1024: Past issue resolved: both timelocks specified check.The handler now correctly rejects requests that specify both
timelock_beforeandtimelock_after, addressing the previous concern about silent ignoring oftimelockAfter.
1220-1233: Past issue resolved: safe JSON error response.The error response for address generation failures now uses proper JSON marshaling with a safe fallback, preventing potential JSON injection from error messages.
🧹 Nitpick comments (2)
bursa.go (2)
1058-1071: Inconsistent error handling for Blake2b initialization.Line 1067 panics if
blake2b.New(28, nil)fails, while Line 1050 returns an error for the identical call. Although blake2b.New with valid parameters should never fail, the inconsistency is confusing.Consider handling both cases consistently—either returning an error from GetScriptHash or documenting why panic is acceptable here (e.g., if GetScriptHash is always called in contexts where panicking is safe).
1163-1175: Structural validation mode has inconsistent signature handling.When
requireSignatures=false(structural validation) and signatures are provided:
- Lines 1169-1171 validate that each signature is exactly 64 bytes
- Line 1174 requires at least one signature
This is inconsistent with the "structural validation" concept. If we're only checking structure (not cryptographic validity), why enforce:
- Signature format (64 bytes)?
- Presence of signatures?
Consider either:
- Ignoring all signature checks when
requireSignatures=false(return true immediately), or- Documenting why format validation is performed even in structural mode
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
bursa.go(4 hunks)bursa_test.go(2 hunks)cmd/bursa/main.go(1 hunks)cmd/bursa/script.go(1 hunks)internal/api/api.go(5 hunks)internal/api/api_test.go(1 hunks)internal/cli/cli.go(2 hunks)openapi.sh(1 hunks)openapi/api/openapi.yaml(5 hunks)openapi/api_default.go(12 hunks)openapi/docs/BursaWallet.md(2 hunks)openapi/model_bursa_wallet.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/api/api_test.go
- openapi/api_default.go
- cmd/bursa/script.go
- cmd/bursa/main.go
🧰 Additional context used
🧬 Code graph analysis (3)
openapi/model_bursa_wallet.go (2)
openapi/model_bursa_key_file.go (1)
BursaKeyFile(22-26)openapi/utils.go (1)
IsNil(335-352)
bursa_test.go (1)
bursa.go (14)
NewScriptSig(945-947)NewScriptAll(950-952)NewScriptAny(955-957)NewScriptNOf(960-962)NewScriptBefore(965-967)NewScriptAfter(970-972)GetScriptHash(1059-1071)GetScriptAddress(1074-1096)ErrInvalidNetwork(47-47)ScriptAll(89-91)MarshalScript(1510-1538)UnmarshalScript(1541-1546)ScriptData(1502-1507)Script(68-73)
bursa.go (1)
bip32/bip32.go (2)
XPrv(43-43)PublicKey(49-49)
⏰ 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). (4)
- GitHub Check: docker
- GitHub Check: nilaway
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (30)
bursa_test.go (12)
18-18: LGTM!The
crypto/ed25519import is correctly added to support the newTestMultiSigScriptFromKeystest that generates Ed25519 key pairs.
882-954: LGTM!Comprehensive test coverage for all six native script types. The test correctly verifies type identifiers (0-5) and CBOR encoding for each variant.
956-961: LGTM!Correctly validates that
GetScriptHashproduces a 28-byte Blake2b-224 hash.
963-974: LGTM!Properly validates mainnet address generation (Bech32
addr1prefix) and error handling for invalid networks.
976-1005: LGTM!Thorough test of CIP-1854 multi-signature key derivation hierarchy, validating the entire chain from root key through account, payment, and stake keys with correct type assertions.
1007-1058: LGTM!Good coverage of script validation logic across all script types, with correct timelock boundary testing. The comments clarify the expected behavior for each case.
1060-1095: LGTM!Comprehensive test of multi-signature script constructors and timelocked script wrapping, with appropriate type and structure validation.
1097-1112: LGTM!Properly tests
NewMultiSigScriptFromKeysby generating Ed25519 keys and validating the resulting script structure.
1114-1137: LGTM!Good edge case coverage validating that invalid constructor parameters produce appropriate errors.
1139-1252: LGTM!Thorough testing of signature validation with correct Ed25519 signature sizes (64 bytes) and proper handling of insufficient vs. sufficient signature counts.
1254-1284: LGTM!Precise boundary testing confirms the exclusive comparison semantics for timelocks:
Beforerequiresslot < boundaryandAfterrequiresslot > boundary.
1286-1361: LGTM!Comprehensive round-trip testing for script serialization/deserialization, including proper error handling for invalid script types and malformed data.
internal/cli/cli.go (4)
18-19: LGTM!The
encoding/hexandencoding/jsonimports are correctly added to support the new script CLI operations.
259-285: LGTM!The
scriptRequiresSignatureshelper correctly traverses the script tree to determine if any component requires signatures, with appropriate handling of all script types.
287-374: LGTM!The
RunScriptValidatefunction properly handles script validation with:
- Script file parsing and unmarshaling
- Hash recomputation for integrity
- Signature parsing and validation
- Appropriate distinction between structural and full validation modes
376-434: LGTM!The
RunScriptAddressfunction correctly:
- Recomputes the script hash from the unmarshaled script
- Generates the network-specific address
- Outputs a consistent JSON response
internal/api/api.go (6)
19-19: LGTM!The
encoding/heximport is correctly added to support hex encoding/decoding in the script API handlers.
143-179: LGTM!Well-structured request/response types for the script API:
ScriptValidateRequestproperly validates signatures format (hex, 128 chars = 64 bytes)ScriptAddressRequestcorrectly requires script and network- Response types align with the CLI output formats
258-261: LGTM!Script routes are correctly registered with appropriate HTTP method handling in the handlers.
909-1046: LGTM!The
handleScriptCreatehandler is well-implemented with:
- Proper request validation using struct tags
- Safe JSON error responses (no injection risk)
- Additional runtime validation for
nOfrequired count- Correct handling of mutually exclusive timelocks
1059-1161: LGTM!The
handleScriptValidatehandler properly:
- Unmarshals scripts and validates their structure
- Computes the script hash from the parsed script (not trusting input)
- Parses and validates signature format
- Returns a consistent validation response
1174-1260: LGTM!The
handleScriptAddresshandler correctly generates script addresses with proper error handling and consistent response format.openapi/docs/BursaWallet.md (2)
7-15: LGTM!The new BursaWallet properties for committee (cold/hot) and DRep keys are correctly documented as optional pointers to
BursaKeyFile, consistent with CIP-1854 governance key support.
45-269: LGTM!Auto-generated accessor documentation follows the established pattern with consistent Get/GetOk/Set/Has methods for all new committee and DRep key fields.
openapi/model_bursa_wallet.go (1)
1-737: LGTM! Generated code follows consistent patterns.This file is auto-generated by OpenAPI Generator (line 10), and the new committee and DRep key fields follow the same accessor/serialization patterns as existing fields. The implementation is consistent and correct for generated code.
openapi/api/openapi.yaml (1)
172-235: LGTM! Validation constraints are appropriate.The added constraints are reasonable:
- Name fields: 1-100 characters prevents empty/excessively long names
- Description: 500 character max is sufficient for descriptions
- Numeric IDs:
maximum: 2147483647(2^31-1) aligns with signed 32-bit integer limits and the ErrInvalidDerivationIndex validation in the Go codebursa.go (4)
67-189: LGTM! Script types are well-structured.The Script interface and implementations are clean and follow best practices:
- CBOR() properly returns errors (addresses previous feedback)
- Composite scripts recursively encode sub-scripts correctly
- Type identifiers follow Cardano native script conventions
851-942: LGTM! CIP-1854 multi-sig derivation is correctly implemented.The multi-signature key derivation functions properly follow CIP-1854:
- Uses path m/1854'/1815'/account' (vs m/1852'/1815'/account' for standard wallets)
- Validates derivation indices < 2^31
- Follows the same patterns as existing key derivation functions
1501-1546: LGTM! Script serialization is well-structured.MarshalScript and UnmarshalScript provide clean JSON serialization with:
- Optional address generation when network is specified
- Consistent script hash generation
- Proper error handling throughout
1686-1750: LGTM! Input validation properly rejects invalid values.The mapToScript function correctly validates:
- Lines 1691-1693: Rejects non-integral
nvalues for nOf scripts- Lines 1699-1705: Enforces
nis between 1 and script count- Lines 1728-1732, 1740-1744: Rejects negative or fractional slot values
This addresses previous review feedback and prevents edge cases.
openapi/api/openapi.yaml
Outdated
| payment_vkey: | ||
| description: description | ||
| type: type | ||
| cborHex: cborHex | ||
| payment_extended_skey: | ||
| description: description | ||
| type: type | ||
| cborHex: cborHex | ||
| stake_address: stake_address | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| payment_skey: | ||
| description: description | ||
| type: type | ||
| cborHex: cborHex | ||
| stake_vkey: | ||
| description: description | ||
| type: type | ||
| cborHex: cborHex | ||
| payment_address: payment_address | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| committee_hot_extended_skey: | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| committee_hot_vkey: | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| stake_skey: | ||
| description: description | ||
| type: type | ||
| cborHex: cborHex | ||
| mnemonic: mnemonic | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| drep_vkey: | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| committee_cold_vkey: | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| payment_extended_skey: | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| drep_skey: | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| stake_address: stake1uy9ggsc9qls4pu9qvyyacwnmr9tt0gzcdt5s0zj4au8qkqc65ge8t | ||
| committee_cold_extended_skey: | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| stake_vkey: | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| committee_cold_skey: | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| payment_address: addr1qxqs59lphg8g6qndelq8xwqn60ag3aeyfcp33c2kdp46a429mg32v29q3cg4sdj5c9ts5vxknc0yutnj8x8r8qy2l2q9sgds2 | ||
| committee_hot_skey: | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| mnemonic: abandon abandon abandon abandon abandon abandon abandon abandon | ||
| abandon abandon abandon about | ||
| stake_extended_skey: | ||
| description: description | ||
| type: type | ||
| cborHex: cborHex | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 | ||
| drep_extended_skey: | ||
| description: Payment Verification Key | ||
| type: PaymentVerificationKeyShelley_ed25519 | ||
| cborHex: 5820a9ebe2e435c03608fbdec443686d23661a796ab6d4dea71734c69b6dde310880 |
There was a problem hiding this comment.
Examples reuse identical values for different key types.
All key fields in the wallet example use the same type: PaymentVerificationKeyShelley_ed25519, description: Payment Verification Key, and cborHex value, which is unrealistic. Different keys should have distinct:
- Type values (e.g.,
StakeVerificationKeyShelley_ed25519forstake_vkey,DRepSigningKeyShelley_ed25519fordrep_skey) - Descriptions (e.g.,
Committee Hot Signing Key,DRep Verification Key) - cborHex values (each key has unique hex)
This may confuse API consumers trying to understand the wallet structure.
wolf31o2
force-pushed
the
feat/cip-1854-multisig-support
branch
from
3c87157 to
ff07c33
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openapi/api/openapi.yaml (1)
254-317: Examples still reuse identical values for different key types.All key fields in the wallet example continue to use the same
type: PaymentVerificationKeyShelley_ed25519,description: Payment Verification Key, andcborHexvalue. Different keys should have distinct type values (e.g.,CommitteeHotSigningKeyShelley_ed25519forcommittee_hot_skey), descriptions, and cborHex values.
🧹 Nitpick comments (5)
docs/swagger.yaml (1)
26-55: Consider adding maxItems constraint to key_hashes array.The
key_hashesarray inScriptCreateRequesthasminItems: 1but nomaxItemsconstraint. Without an upper bound, a malicious client could potentially send thousands of key hashes, causing excessive memory usage or processing time.Consider adding a reasonable maximum (e.g., 255 signers) both in this OpenAPI spec and in the corresponding Go struct validation tag in
internal/api/api.goline 137.Based on static analysis hints.
docs/swagger.json (1)
22-64: Align script schema field naming and enums for consistency.The three script endpoints and their models look coherent, but a few naming/details would be clearer if aligned:
api.ScriptCreateRequest.network/api.ScriptAddressRequest.network/api.ScriptValidateRequest.networkare enums, whileapi.ScriptAddressResponse.networkis a free-form string. Consider also declaring it as"enum": ["mainnet","testnet"]for symmetry.api.ScriptResponseexposeshash, while the validate and address responses exposescriptHash. Using a single name (scriptHashorhash) across all script responses would make the API easier to consume.- In
api.ScriptValidateRequest,signaturesis an array of hex strings, butapi.ScriptValidateResponse.signaturesis an integer count. That’s valid, but mildly surprising; renaming the response field to something likesignatureCount(or adding description text in the schema) would reduce ambiguity.These are surface-level adjustments but easier to change now before external clients depend on them.
Also applies to: 65-107, 108-150, 362-504
internal/cli/cli.go (2)
126-213: Script creation flag validation and construction logic look solid (one small naming nit).The mutual-exclusion checks for
--all/--any/--required, the non-empty key-hash check, therequired <= len(keyHashes)guard, and the explicittimelockBefore/timelockAfterconflict check are all good and prevent impossible or panicking script configurations. The selection ofNewAllMultiSigScript/NewAnyMultiSigScript/NewMultiSigScriptplus the optionalNewTimelockedScriptwrapper matches the underlying library’s constructors correctly.Minor:
any boolas a parameter name shadows Go’s predeclaredanytype; consider renaming it touseAnyor similar to avoid confusion in call sites and tooling.
371-421: Address generation CLI is correct; consider slightly clearer network error messaging.
RunScriptAddressmirrorsRunScriptValidate’s input handling, recomputesscriptHashfrom the unmarshaled script, and callsGetScriptAddress(script, network)so invalid/empty networks will surface as errors from the library. As a small UX improvement, you might want to include thenetworkvalue in the error log when address generation fails, to make misconfigured invocations easier to diagnose.Example:
logger.Error("failed to generate script address", "network", network, "error", err)bursa_test.go (1)
1139-1252: Clarify that “requireSignatures” validation is structural (count/length) rather than cryptographic.
TestValidateScriptWithSignaturestreats any 64-byte slice as a “valid” signature and asserts success once the expected number of 64-byte entries is present. Given thatValidateScripthas no access to the transaction body or public keys, it cannot actually perform Ed25519 verification; in practice it’s enforcing structural conditions (enough signatures of the right length), not cryptographic correctness.If that is the intended contract, consider making this explicit in documentation and/or naming (for example, documenting
requireSignaturesas “enforce signature presence and shape constraints only”) so API/CLI consumers don’t mistake these checks for full on-chain-style signature validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
bursa.go(5 hunks)bursa_test.go(2 hunks)cmd/bursa/main.go(1 hunks)cmd/bursa/script.go(1 hunks)docs/docs.go(3 hunks)docs/swagger.json(3 hunks)docs/swagger.yaml(2 hunks)internal/api/api.go(5 hunks)internal/api/api_test.go(1 hunks)internal/cli/cli.go(2 hunks)openapi.sh(1 hunks)openapi/api/openapi.yaml(5 hunks)openapi/docs/BursaWallet.md(2 hunks)openapi/model_bursa_wallet.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- openapi/docs/BursaWallet.md
- openapi.sh
- internal/api/api_test.go
🧰 Additional context used
🧬 Code graph analysis (5)
internal/cli/cli.go (2)
internal/logging/logging.go (1)
GetLogger(78-83)bursa.go (15)
Script(68-73)NewAllMultiSigScript(997-1008)NewAnyMultiSigScript(1011-1022)NewMultiSigScript(976-994)NewTimelockedScript(1025-1036)MarshalScript(1502-1530)ScriptSig(76-78)ScriptAll(89-91)ScriptAny(114-116)ScriptNOf(139-142)ScriptBefore(165-167)ScriptAfter(178-180)ScriptData(1494-1499)GetScriptHash(1059-1071)GetScriptAddress(1074-1096)
internal/api/api.go (2)
bursa.go (11)
Script(68-73)NewMultiSigScript(976-994)NewAllMultiSigScript(997-1008)NewAnyMultiSigScript(1011-1022)NewTimelockedScript(1025-1036)MarshalScript(1502-1530)UnmarshalScript(1533-1538)ScriptData(1494-1499)ValidateScript(1101-1135)GetScriptHash(1059-1071)GetScriptAddress(1074-1096)internal/logging/logging.go (1)
GetLogger(78-83)
openapi/model_bursa_wallet.go (2)
openapi/model_bursa_key_file.go (1)
BursaKeyFile(22-26)openapi/utils.go (1)
IsNil(335-352)
bursa.go (1)
bip32/bip32.go (2)
XPrv(43-43)PublicKey(49-49)
bursa_test.go (1)
bursa.go (23)
NewScriptSig(945-947)NewScriptAll(950-952)NewScriptAny(955-957)NewScriptNOf(960-962)NewScriptBefore(965-967)NewScriptAfter(970-972)GetScriptHash(1059-1071)GetScriptAddress(1074-1096)ErrInvalidNetwork(47-47)GetMultiSigAccountKey(852-862)GetMultiSigPaymentKey(865-873)GetMultiSigPaymentVKey(876-893)GetMultiSigPaymentSKey(896-902)GetMultiSigStakeKey(905-913)GetMultiSigStakeVKey(916-933)GetMultiSigStakeSKey(936-942)ValidateScript(1101-1135)NewTimelockedScript(1025-1036)ScriptAll(89-91)MarshalScript(1502-1530)UnmarshalScript(1533-1538)ScriptData(1494-1499)Script(68-73)
🪛 Checkov (3.2.334)
docs/swagger.yaml
[medium] 29-34: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (10)
internal/api/api.go (2)
133-141: LGTM!The request validation is well-structured with appropriate constraints for script creation. The
Requiredfield validation has been properly fixed to includeomitemptyto avoid false positives forall/anyscript types.
1007-1024: Good fix for timelock validation.The code now properly rejects requests with both
timelock_beforeandtimelock_after, addressing the previous review concern about silent behavior.bursa.go (1)
68-188: LGTM!The Script interface and implementations are well-designed. The CBOR error handling has been properly addressed from previous reviews, with all methods now returning errors appropriately.
cmd/bursa/script.go (1)
99-143: LGTM!The help text examples now correctly demonstrate structural validation with the
--structural-onlyflag for timelock-only scripts (line 120), addressing the previous review concern about misleading documentation.docs/swagger.json (1)
600-611: KeyFile schema simplification looks good.Dropping JSON
examplemetadata frombursa.KeyFilefields while keeping types unchanged is safe for clients and keeps the schema focused.internal/cli/cli.go (3)
214-248: Nice improvement on output file handling.Explicitly checking
file.Close()in the deferred function ensures I/O errors on close aren’t silently ignored after a successfulEncode, which is important for correctness when writing script JSON to disk.
260-280: RecursivescriptRequiresSignatureshelper is correct and readable.Using
slices.ContainsFuncwith a recursive predicate overScriptAll/ScriptAny/ScriptNOfand treatingScriptBefore/ScriptAfteras non-signature-bearing cleanly captures “does any subtree require signatures?”. This matches how it’s consumed inRunScriptValidateto decide whether signatures are mandatory when--structural-onlyis false.
282-369: Script validate CLI behavior and output are well-structured.
RunScriptValidatecorrectly: readsScriptData, unmarshals viaUnmarshalScript, recomputes the hash from the parsed script (avoiding stale/forgedhashfields), decodes signatures from hex, enforces presence of signatures when the script tree contains anysignodes and--structural-onlyis not set, and delegates toValidateScriptwithrequireSignatures = !structuralOnly. The JSON result (valid/slot/signature count/scriptHash/structuralOnly) is straightforward for downstream tooling.No changes needed from my side here.
bursa_test.go (2)
882-975: Good coverage of script constructors, hashing, address, and multisig builders.The new tests around
ScriptSig/ScriptAll/ScriptAny/ScriptNOf/ScriptBefore/ScriptAftertype IDs and CBOR,GetScriptHashlength, script-address generation (including invalid network handling), multisig key derivation, high-levelValidateScriptbehavior, and the various multisig-constructor helpers (NewMultiSigScript,NewAllMultiSigScript,NewAnyMultiSigScript,NewTimelockedScript,NewMultiSigScriptFromKeys) collectively give strong coverage of the native-script surface. The edge-case tests for invalid parameter combinations (e.g.,requiredout of range, empty key sets) also match the underlying constructor contracts.No issues spotted in these tests.
Also applies to: 976-1005, 1007-1095, 1097-1137
1286-1361: Round‑trip and error‑path tests for ScriptData look correct.
TestScriptRoundTripexercisesMarshalScript/UnmarshalScriptfor bothScriptSigandScriptAll, and also covers invalidTypevalues and badnvalues fornOfscripts, ensuring deserialization fails cleanly in those cases. This gives good confidence in the JSON <-> internal script conversion layer.
wolf31o2
force-pushed
the
feat/cip-1854-multisig-support
branch
2 times, most recently
from
ef8e5c3 to
c8bfadf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
internal/api/api_test.go (1)
734-818: Good negative coverage for script validate error casesThe table‑driven tests cover malformed keyHash, missing fields, invalid network, and unsupported script type, all asserting on 400 and an error substring. This matches the expected behavior and previous review feedback about clarifying the first case name (“invalid keyHash format”).
cmd/bursa/script.go (1)
99-143: script validate CLI help now matches validation behaviorThe long help correctly distinguishes full validation vs
--structural-only, and the examples now include--structural-onlyfor timelock‑only validation, aligning withRunScriptValidate’s enforcement of signatures. This addresses the earlier review concern about misleading examples.internal/cli/cli.go (1)
126-258: RunScriptCreate validates inputs thoroughly and handles output robustlyThe create path now:
- Enforces mutual exclusivity of
--all/--any/--requiredand ensures at least one mode is chosen.- Requires at least one key hash and that
required <= len(keyHashes).- Rejects conflicting timelocks (
--timelock-beforeand--timelock-afterset together).- Parses key hashes via
hex.DecodeStringand wraps the resulting script with an optional timelock.- Uses
MarshalScriptso hash and (optional) address are recomputed, and writes JSON either to a file (with checkedClose) or stdout.This is a solid implementation and addresses previous review concerns about validation and hash trust.
openapi/api/openapi.yaml (1)
456-520: Wallet example still uses identical values for different key types.All key fields in the wallet example use the same placeholder values (
description: description,type: type,cborHex: cborHex), which doesn't demonstrate the actual distinct values each key type should have. This was previously flagged.bursa.go (1)
916-939: Inconsistentnvalidation:NewScriptNOfallowsn=0butmapToScriptrequiresn>=1.
NewScriptNOfacceptsn=0(line 918:n < 0), butmapToScriptrejects it (line 1691:n < 1). A 0-of-N script is always satisfied regardless of signatures, which may not be the intended behavior. Align the validation:func NewScriptNOf(n int, scripts ...Script) *NativeScript { - if n < 0 || n > len(scripts) { + if n < 1 || n > len(scripts) { return nil }
🧹 Nitpick comments (11)
docs/swagger.yaml (1)
29-32: Consider adding maxItems constraint to key_hashes array.While
minItems: 1ensures at least one key hash, adding amaxItemsconstraint would prevent potential abuse and align with practical multi-signature limits. Consider setting a reasonable maximum (e.g., 15-20) based on your use case.key_hashes: items: type: string minItems: 1 + maxItems: 20 type: arrayinternal/api/api_test.go (2)
658-691: Script create handler test is minimal but correctThe happy‑path coverage for
/api/script/createis sound (status 200 and basic field presence). If you want stronger regression protection later, consider parsing the JSON into a typedApiScriptResponseand asserting field types/shape instead of stringContains, but this is fine for now.
820-855: Script address handler test correctly asserts key response propertiesThe test validates status 200 and checks for
address,network,scriptHash, plus anaddr1prefix, which is a reasonable smoke test for address generation. If you later expose testnet addresses, you might want an additional case asserting the testnet prefix, but current coverage is sufficient.cmd/bursa/script.go (1)
36-97: script create CLI maps cleanly onto RunScriptCreateFlags and examples are coherent with
RunScriptCreate’s signature (required,key-hashes,all,any, timelocks,network,output). One minor polish you might consider is using 56‑hex‑char examples to match actual Cardano key hash length, but that’s cosmetic.internal/cli/cli.go (1)
260-296: scriptRequiresSignatures is correct but can be simplified to avoid allocationsThe recursive detection over
NativeScriptAll/Any/NofKis logically sound, but creating intermediate[]bursa.Scriptslices just to useslices.ContainsFuncadds avoidable allocations.You could iterate directly over
s.Scriptsand recurse:- case *bursa.NativeScriptAll: - scripts := make([]bursa.Script, len(s.Scripts)) - for i, scr := range s.Scripts { - scripts[i] = scr - } - if slices.ContainsFunc(scripts, scriptRequiresSignatures) { - return true - } + case *bursa.NativeScriptAll: + for _, scr := range s.Scripts { + if scriptRequiresSignatures(scr) { + return true + } + }(and similarly for
NativeScriptAnyandNativeScriptNofK). Behavior stays the same with less overhead.openapi/model_api_script_validate_response.go (1)
18-237: ApiScriptValidateResponse matches the expected JSON schemaThe generated model correctly exposes optional
scriptHash,signatures,slot, andvalidfields with the usual accessors andMappedNullableimplementation. For nowint32forslotandsignaturesis adequate; if you ever move the OpenAPI spec toint64for slots, this file will naturally regenerate accordingly.internal/api/api.go (1)
1109-1125: Consider removing signature values from error logs.The error logging at lines 1112-1118 includes the signature string value. While this is an error path for invalid hex format, logging signature data (even malformed) could be a concern in security-sensitive contexts. Consider logging only the index or a sanitized indicator.
sig, err := hex.DecodeString(sigStr) if err != nil { logger.Error( "invalid signature format", - "signature", - sigStr, + "index", + i, "error", err, )openapi/docs/ApiScriptResponse.md (1)
87-92: Minor doc inconsistency:GetScriptOkreturn type.The documentation shows
(*map[string]interface{}, bool)but the actual implementation inmodel_api_script_response.goreturns(map[string]interface{}, bool). This is auto-generated, so regenerating the docs may fix it.bursa.go (3)
857-872: Script constructors silently returnnilon error.
NewScriptSig(and similar constructors likeNewScriptAll,NewScriptAny,NewScriptNOf,NewScriptBefore,NewScriptAfter) returnnilwhen CBOR encoding/decoding fails, with no error indication. Callers cannot distinguish between invalid input and internal failures.Consider returning
(*NativeScript, error)to allow callers to handle failures:-func NewScriptSig(keyHash []byte) *NativeScript { +func NewScriptSig(keyHash []byte) (*NativeScript, error) { concrete := &NativeScriptPubkey{ Type: 0, Hash: keyHash, } cborData, err := cbor.Encode(concrete) if err != nil { - return nil + return nil, fmt.Errorf("failed to encode script sig: %w", err) } var script NativeScript if _, err := cbor.Decode(cborData, &script); err != nil { - return nil + return nil, fmt.Errorf("failed to decode script sig: %w", err) } - return &script + return &script, nil }
1111-1114:ValidateScriptsilently returnsfalsefor non-pointer script types.If a caller passes a value type instead of
*NativeScript, the function returnsfalsewithout indicating why. Since all constructors return pointers, this may not be a practical issue, but consider adding documentation or returning an error for invalid inputs.
1634-1677: Consider validating non-emptyscriptsarray forall/anytypes.The
mapToScriptfunction accepts emptyscriptsarrays forallandanytypes. Anallscript with zero sub-scripts trivially passes (vacuous truth), while ananyscript with zero sub-scripts always fails. Consider adding validation:case "all": scriptsInterface, ok := m["scripts"].([]any) if !ok { return nil, errors.New("missing or invalid scripts for all script") } + if len(scriptsInterface) == 0 { + return nil, errors.New("all script requires at least one sub-script") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
bursa.go(5 hunks)bursa_test.go(2 hunks)cmd/bursa/main.go(1 hunks)cmd/bursa/script.go(1 hunks)docs/docs.go(3 hunks)docs/swagger.json(3 hunks)docs/swagger.yaml(2 hunks)internal/api/api.go(5 hunks)internal/api/api_test.go(1 hunks)internal/cli/cli.go(2 hunks)openapi.sh(1 hunks)openapi/.openapi-generator/FILES(2 hunks)openapi/README.md(2 hunks)openapi/api/openapi.yaml(6 hunks)openapi/api_default.go(14 hunks)openapi/docs/ApiScriptAddressRequest.md(1 hunks)openapi/docs/ApiScriptAddressResponse.md(1 hunks)openapi/docs/ApiScriptCreateRequest.md(1 hunks)openapi/docs/ApiScriptResponse.md(1 hunks)openapi/docs/ApiScriptValidateRequest.md(1 hunks)openapi/docs/ApiScriptValidateResponse.md(1 hunks)openapi/docs/BursaWallet.md(2 hunks)openapi/docs/DefaultAPI.md(2 hunks)openapi/model_api_script_address_request.go(1 hunks)openapi/model_api_script_address_response.go(1 hunks)openapi/model_api_script_create_request.go(1 hunks)openapi/model_api_script_response.go(1 hunks)openapi/model_api_script_validate_request.go(1 hunks)openapi/model_api_script_validate_response.go(1 hunks)openapi/model_bursa_wallet.go(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- openapi/docs/ApiScriptCreateRequest.md
- openapi/docs/ApiScriptAddressResponse.md
🧰 Additional context used
🧬 Code graph analysis (10)
cmd/bursa/script.go (1)
internal/cli/cli.go (4)
Run(32-34)RunScriptCreate(126-258)RunScriptValidate(298-385)RunScriptAddress(387-451)
openapi/model_api_script_validate_response.go (2)
openapi/utils.go (2)
MappedNullable(354-356)IsNil(335-352)bursa.go (1)
GetScriptHash(1063-1075)
openapi/model_api_script_create_request.go (1)
openapi/utils.go (2)
MappedNullable(354-356)IsNil(335-352)
openapi/model_api_script_address_response.go (1)
openapi/utils.go (2)
MappedNullable(354-356)IsNil(335-352)
internal/cli/cli.go (2)
internal/logging/logging.go (1)
GetLogger(78-83)bursa.go (17)
Script(69-69)NewAnyMultiSigScript(1015-1026)NewMultiSigScript(977-998)NewTimelockedScript(1029-1040)MarshalScript(1507-1535)NativeScript(73-73)NativeScriptPubkey(74-74)NativeScriptAll(75-75)NativeScriptAny(76-76)NativeScriptNofK(77-77)NativeScriptInvalidBefore(78-78)NativeScriptInvalidHereafter(79-79)ScriptData(1499-1504)UnmarshalScript(1538-1543)GetScriptHash(1063-1075)ValidateScript(1105-1132)GetScriptAddress(1078-1100)
internal/api/api.go (2)
bursa.go (9)
Script(69-69)NewMultiSigScript(977-998)NewAllMultiSigScript(1001-1012)NewAnyMultiSigScript(1015-1026)NewTimelockedScript(1029-1040)MarshalScript(1507-1535)UnmarshalScript(1538-1543)ScriptData(1499-1504)GetScriptAddress(1078-1100)internal/logging/logging.go (1)
GetLogger(78-83)
openapi/model_api_script_address_request.go (2)
openapi/utils.go (1)
MappedNullable(354-356)bursa.go (1)
Script(69-69)
openapi/model_api_script_response.go (1)
openapi/utils.go (2)
MappedNullable(354-356)IsNil(335-352)
bursa.go (1)
bip32/bip32.go (2)
XPrv(43-43)PublicKey(49-49)
openapi/model_bursa_wallet.go (2)
openapi/model_bursa_key_file.go (1)
BursaKeyFile(22-26)openapi/utils.go (1)
IsNil(335-352)
🪛 Checkov (3.2.334)
docs/swagger.yaml
[medium] 29-34: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 LanguageTool
openapi/docs/ApiScriptValidateRequest.md
[grammar] ~52-~52: Ensure spelling is correct
Context: ...ets Network field to given value. ### GetRequireSignatures func (o *ApiScriptValidateRequest) GetRequireSignatures() bool GetRequireSignatures returns the Require...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ... if non-nil, zero value otherwise. ### GetRequireSignaturesOk func (o *ApiScriptValidateRequest) GetRequireSignaturesOk() (*bool, bool) GetRequireSignaturesOk returns a tuple w...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (37)
openapi/model_bursa_wallet.go (1)
23-40: LGTM: Generated model expansion follows established patterns.The new Committee and DRep key fields are properly structured as optional pointers to
BursaKeyFile, consistent with existing Payment and Stake key fields. The generated accessors and serialization logic correctly handle nil values.cmd/bursa/main.go (1)
42-42: LGTM: Script subcommand properly integrated.The
scriptCommand()registration follows the same pattern as existing commands and correctly wires up the new CLI functionality.openapi.sh (1)
5-5: LGTM: Portability issue resolved.The OS-specific handling for
sed -iproperly addresses the macOS vs Linux portability concerns raised in previous reviews. The$OSTYPEcheck cleanly handles both platforms without requiring backup files.openapi/README.md (1)
82-100: LGTM: Documentation properly updated for new script endpoints.The README correctly documents all three new script API endpoints and their associated request/response models, maintaining consistency with the existing documentation structure.
openapi/docs/DefaultAPI.md (1)
19-215: LGTM: Comprehensive API endpoint documentation.The documentation for the three new script endpoints (address, create, validate) is thorough and follows the established format, including usage examples and complete parameter descriptions.
openapi/docs/ApiScriptValidateRequest.md (1)
1-149: LGTM: Model documentation is complete and accurate.The documentation thoroughly covers all properties and methods for
ApiScriptValidateRequest, clearly indicating required vs optional fields.openapi/docs/ApiScriptValidateResponse.md (1)
1-134: LGTM: Response model documentation is thorough.The documentation clearly describes all response properties and their accessor methods, maintaining consistency with the request model documentation.
docs/swagger.yaml (2)
3-100: LGTM: Well-structured API schemas for script operations.The new schema definitions properly use enums for validation, mark required fields, and leverage
additionalProperties: truefor flexible script objects. The design aligns well with the multi-signature use case.
232-315: LGTM: Script endpoints are well-documented.All three endpoints (
/api/script/address,/api/script/create,/api/script/validate) have clear descriptions, proper request/response schema references, and appropriate HTTP status codes (200, 400, 500).openapi/docs/ApiScriptAddressRequest.md (1)
1-72: ApiScriptAddressRequest doc looks consistent with generated modelContent and method signatures match the standard OpenAPI generator pattern for a request with
NetworkandScriptfields; nothing to fix here.internal/api/api_test.go (1)
693-732: Script validate handler test verifies both transport and semantic resultThis test exercises the validate handler end‑to‑end and explicitly asserts
valid == truefrom the decoded JSON, which is exactly what you want here. No issues.cmd/bursa/script.go (2)
22-34: script root command wiring is straightforward and correctThe
scriptgroup command is minimal and correctly registers the three subcommands; no issues here.
145-177: script address CLI is correctly parameterizedThe address command requires
--scriptand passes bothscriptFileandnetworktoRunScriptAddress, with sensible defaults and examples. No functional issues.bursa_test.go (4)
882-977: Core script type, hash, and address tests align with implementationThe tests for
NewScriptSig/All/Any/NOf/Before/After,GetScriptHash(28‑byte Blake2b‑224), andGetScriptAddress(including invalid network error) closely track the library’s advertised behavior. Timelock type ids and address prefix checks (addr1) look correct for current semantics.
978-1118: Multisig key derivation and script construction have solid coverage
TestMultiSigKeyDerivation,TestMultiSigScriptGeneration, andTestMultiSigScriptFromKeysvalidate CIP‑1854 key derivation, type tags on key files, and the structure of N‑of‑K/All/Any/timelocked scripts. Using real ed25519 public keys forNewMultiSigScriptFromKeysis appropriate and should remain stable across runs.
1120-1293: Signature and timelock validation tests encode the intended semantics
TestScriptGenerationEdgeCasesasserts proper error conditions for invalid multisig parameters.TestValidateScriptWithSignaturesclearly documents the current “structural only” interpretation ofrequireSignaturesand enforces signature count/length per script shape, whileTestValidateScriptTimelockBoundariescorrectly encodes<forScriptBeforeand>=forScriptAfterin line with Cardano native script semantics. These are valuable guardrails.
1295-1374: Round‑trip and invalid‑data tests for script (un)marshalling look good
TestScriptRoundTripcoversMarshalScript/UnmarshalScriptfor bothScriptSigandScriptAll, and explicitly checks failure cases for unsupportedTypeand badnvalues in N‑of‑K. This should catch regressions in the JSON <-> internal representation mapping.internal/cli/cli.go (2)
298-385: RunScriptValidate matches CLI semantics and recomputes script hashThe validate helper:
- Opens and decodes
ScriptDataJSON, then unmarshals to a script.- Recomputes the hash from the script CBOR, avoiding trusting any hash in the file.
- Decodes signatures from hex and enforces presence only when
structuralOnly == falseand the script tree contains pubkey requirements (scriptRequiresSignatures).- Delegates to
bursa.ValidateScriptwithrequireSignatures = !structuralOnlyand emits a concise JSON result.This cleanly implements the documented behavior for full vs structural validation.
387-451: RunScriptAddress correctly derives address and hash from the scriptRunScriptAddress:
- Reads
ScriptData, unmarshals to the internal script.- Recomputes the script hash.
- Calls
GetScriptAddresswith the provided network and outputs JSON containingaddress,network, andscriptHash.Error handling is consistent with the rest of the CLI (logging and
os.Exit(1)), and it doesn’t rely on any precomputed hash or address in the file, which is the right choice.openapi/.openapi-generator/FILES (1)
8-29: OpenAPI generator file list correctly includes new script models/docsThe added entries for script request/response docs and models are consistent with the new OpenAPI types and endpoints; no changes needed.
openapi/model_api_script_create_request.go (1)
20-330: ApiScriptCreateRequest generation and validation logic are appropriateThe model’s fields, JSON tags (
key_hashes,network,required,timelock_*,type), required‑property checks, andDisallowUnknownFieldsdecoding all align with how the API tests build create requests. This looks correct and doesn’t need manual adjustment.docs/docs.go (1)
1-699: LGTM - Auto-generated Swagger documentation.This file is generated by swaggo/swag and correctly reflects the new script API endpoints and their request/response schemas.
docs/swagger.json (1)
1-677: LGTM - Auto-generated Swagger JSON specification.This file mirrors the Go documentation template and correctly documents the new script API surface.
openapi/model_api_script_address_response.go (1)
1-199: LGTM - Auto-generated OpenAPI model.This file follows the standard OpenAPI Generator patterns for response models with proper nullable handling and JSON serialization.
openapi/model_api_script_address_request.go (1)
1-193: LGTM - Auto-generated OpenAPI model.This file correctly implements the request model with required field validation during JSON unmarshaling and strict parsing via
DisallowUnknownFields().internal/api/api.go (5)
133-179: LGTM - Well-structured request/response types with proper validation.The validation tags are correctly configured:
Requiredfield usesomitemptyto skipmin=1validation forall/anytypes (addressing previous review feedback)KeyHashesvalidates 56 hex characters (28-byte Blake2b-224 hash)Signaturesvalidates 128 hex characters (64-byte Ed25519 signature)
258-261: LGTM - Script API routes properly registered.The three script endpoints are correctly wired to their respective handlers.
1007-1024: Good fix for timelock mutual exclusivity.The check for both
TimelockBeforeandTimelockAfterbeing set (lines 1007-1018) correctly addresses the previous review feedback about silently ignoring one of the timelocks.
1221-1233: LGTM - Safe error response handling.The error response at lines 1226-1231 correctly uses
json.Marshalto safely encode the error message, addressing the previous review feedback about JSON injection.
535-540: LGTM - Safe JSON error encoding pattern.The error response correctly uses
json.Marshalwith a fallback, ensuring error messages containing special characters don't cause JSON injection or malformed responses.openapi/model_api_script_response.go (1)
1-235: Auto-generated OpenAPI model looks correct.This is standard OpenAPI Generator output with proper accessor patterns, nullable wrapper, and JSON serialization. No issues identified.
bursa.go (4)
1167-1183: Signature validation is documented as placeholder; ensure tests reflect this.The implementation checks signature count but doesn't track which signatures satisfy which sub-scripts. A single valid signature could satisfy multiple
ScriptSigchildren. This is documented as placeholder behavior (per past review comments), but ensure integration tests clearly verify this limitation before production use.
762-855: CIP-1854 multi-sig key derivation looks correct.The derivation path
1854'/1815'/account'/role/indexfollows CIP-1854 specification. Key file creation reuses existing helpers appropriately.
1062-1100: Script hashing and address generation implementation is correct.
GetScriptHashproperly uses Blake2b-224 on raw script bytes, andGetScriptAddresscorrectly creates a script address with no stake part usingAddressTypeScriptNone.
1224-1235: No issues identified. The timelock validation logic is correct and intentionally uses user-facing semantics ("before" = valid before, "after" = valid after) rather than Cardano's inverted naming convention. Test cases confirm the validation logic produces the expected behavior.openapi/api_default.go (1)
1-683: Auto-generated code: LGTMThis file is marked as generated by OpenAPI Generator and follows the "DO NOT EDIT" directive. The new script endpoints (
ApiScriptAddressPost,ApiScriptCreatePost,ApiScriptValidatePost) follow the same established patterns as the existing wallet endpoints, with consistent:
- Request body validation
- Content-Type/Accept header handling
- Error response handling for 400/500 status codes
- Response decoding
Any adjustments should be made in the source OpenAPI spec (
openapi.yaml), not in this generated file.openapi/docs/BursaWallet.md (1)
7-15: Auto-generated documentation: LGTMThe new committee and DRep key fields are properly documented as optional
BursaKeyFilepointers, consistent with other key fields in the model. The documentation follows the established patterns with complete Get/GetOk/Set/Has method documentation for each new field.
wolf31o2
force-pushed
the
feat/cip-1854-multisig-support
branch
from
c8bfadf to
22f1fe2
Compare
|
@cubic-dev-ai review this PR |
@wolf31o2 I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
9 issues found across 30 files
Prompt for AI agents (all 9 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="internal/api/api.go">
<violation number="1" location="internal/api/api.go:146">
P2: The `Network` field in `ScriptValidateRequest` is marked as required but is never used in `handleScriptValidate`. This forces API consumers to provide a value that serves no purpose. Consider either removing the `required` validation if the field is for future use, or removing the field entirely from this request struct.</violation>
</file>
<file name="docs/swagger.json">
<violation number="1" location="docs/swagger.json:422">
P2: The `timelock_after` and `timelock_before` fields should include `"format": "int64"` like the `slot` field, since they represent blockchain slots which can be large numbers.</violation>
<violation number="2" location="docs/swagger.json:444">
P2: Inconsistent field naming: `ScriptResponse` uses `hash` while `ScriptAddressResponse` and `ScriptValidateResponse` use `scriptHash`. Consider using `scriptHash` consistently across all response schemas for better API consistency and client code generation.</violation>
</file>
<file name="bursa.go">
<violation number="1" location="bursa.go:878">
P1: Unsafe type assertion will panic if script is not a `*NativeScript`. Use checked assertion with `, ok` pattern and handle the error case.</violation>
<violation number="2" location="bursa.go:994">
P2: `NewScriptSig` can return nil on error, but this is not checked before passing to `NewScriptNOf`, which will panic when dereferencing.</violation>
</file>
<file name="internal/cli/cli.go">
<violation number="1" location="internal/cli/cli.go:248">
P2: Misleading user output: success message is logged before deferred `file.Close()`, so if close fails, users see success message but program exits with error code 1. Consider moving the success log after explicitly closing the file, or handle the close error before logging success.</violation>
</file>
<file name="docs/swagger.yaml">
<violation number="1" location="docs/swagger.yaml:41">
P1: Missing `format: int64` for `timelock_after` field. This field represents a Cardano slot number which can exceed 32-bit integer limits. The `slot` field in `ScriptValidateRequest` correctly uses `format: int64`, but this field generates as `int32`, causing potential overflow.</violation>
<violation number="2" location="docs/swagger.yaml:43">
P1: Missing `format: int64` for `timelock_before` field. Like `timelock_after`, this represents a Cardano slot number and should use 64-bit integers to avoid overflow.</violation>
</file>
<file name="openapi/api/openapi.yaml">
<violation number="1" location="openapi/api/openapi.yaml:405">
P2: Missing `minimum: 0` constraint for derivation index fields. These BIP-32 derivation path indices must be non-negative (0 to 2147483647). Without this validation, the API would accept invalid negative values.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
internal/api/api.go
Outdated
| // ScriptValidateRequest defines the request payload for script validation | ||
| type ScriptValidateRequest struct { | ||
| Script map[string]any `json:"script" validate:"required"` | ||
| Network string `json:"network" validate:"required,oneof=mainnet testnet"` |
There was a problem hiding this comment.
P2: The Network field in ScriptValidateRequest is marked as required but is never used in handleScriptValidate. This forces API consumers to provide a value that serves no purpose. Consider either removing the required validation if the field is for future use, or removing the field entirely from this request struct.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/api/api.go, line 146:
<comment>The `Network` field in `ScriptValidateRequest` is marked as required but is never used in `handleScriptValidate`. This forces API consumers to provide a value that serves no purpose. Consider either removing the `required` validation if the field is for future use, or removing the field entirely from this request struct.</comment>
<file context>
@@ -129,6 +130,54 @@ type WalletUpdateRequest struct {
+// ScriptValidateRequest defines the request payload for script validation
+type ScriptValidateRequest struct {
+ Script map[string]any `json:"script" validate:"required"`
+ Network string `json:"network" validate:"required,oneof=mainnet testnet"`
+ Signatures []string `json:"signatures,omitempty" validate:"dive,hexadecimal,len=128"`
+ Slot uint64 `json:"slot,omitempty" swaggertype:"integer" format:"int64"`
</file context>
docs/swagger.json
Outdated
| "address": { | ||
| "type": "string" | ||
| }, | ||
| "hash": { |
There was a problem hiding this comment.
P2: Inconsistent field naming: ScriptResponse uses hash while ScriptAddressResponse and ScriptValidateResponse use scriptHash. Consider using scriptHash consistently across all response schemas for better API consistency and client code generation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/swagger.json, line 444:
<comment>Inconsistent field naming: `ScriptResponse` uses `hash` while `ScriptAddressResponse` and `ScriptValidateResponse` use `scriptHash`. Consider using `scriptHash` consistently across all response schemas for better API consistency and client code generation.</comment>
<file context>
@@ -230,6 +359,151 @@
+ "address": {
+ "type": "string"
+ },
+ "hash": {
+ "type": "string"
+ },
</file context>
| "type": "integer", | ||
| "minimum": 1 | ||
| }, | ||
| "timelock_after": { |
There was a problem hiding this comment.
P2: The timelock_after and timelock_before fields should include "format": "int64" like the slot field, since they represent blockchain slots which can be large numbers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/swagger.json, line 422:
<comment>The `timelock_after` and `timelock_before` fields should include `"format": "int64"` like the `slot` field, since they represent blockchain slots which can be large numbers.</comment>
<file context>
@@ -230,6 +359,151 @@
+ "type": "integer",
+ "minimum": 1
+ },
+ "timelock_after": {
+ "type": "integer"
+ },
</file context>
bursa.go
Outdated
|
|
||
| scripts := make([]Script, len(keyHashes)) | ||
| for i, keyHash := range keyHashes { | ||
| scripts[i] = NewScriptSig(keyHash) |
There was a problem hiding this comment.
P2: NewScriptSig can return nil on error, but this is not checked before passing to NewScriptNOf, which will panic when dereferencing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bursa.go, line 994:
<comment>`NewScriptSig` can return nil on error, but this is not checked before passing to `NewScriptNOf`, which will panic when dereferencing.</comment>
<file context>
@@ -724,6 +759,481 @@ func GetCommitteeHotExtendedSKey(committeeKey bip32.XPrv) (KeyFile, error) {
+
+ scripts := make([]Script, len(keyHashes))
+ for i, keyHash := range keyHashes {
+ scripts[i] = NewScriptSig(keyHash)
+ }
+
</file context>
bursa.go
Outdated
| func NewScriptAll(scripts ...Script) *NativeScript { | ||
| nativeScripts := make([]NativeScript, len(scripts)) | ||
| for i, script := range scripts { | ||
| nativeScripts[i] = *script.(*NativeScript) |
There was a problem hiding this comment.
P1: Unsafe type assertion will panic if script is not a *NativeScript. Use checked assertion with , ok pattern and handle the error case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bursa.go, line 878:
<comment>Unsafe type assertion will panic if script is not a `*NativeScript`. Use checked assertion with `, ok` pattern and handle the error case.</comment>
<file context>
@@ -724,6 +759,481 @@ func GetCommitteeHotExtendedSKey(committeeKey bip32.XPrv) (KeyFile, error) {
+func NewScriptAll(scripts ...Script) *NativeScript {
+ nativeScripts := make([]NativeScript, len(scripts))
+ for i, script := range scripts {
+ nativeScripts[i] = *script.(*NativeScript)
+ }
+ concrete := &NativeScriptAll{
</file context>
| logger.Error("failed to write script to file", "error", err) | ||
| os.Exit(1) | ||
| } | ||
| logger.Info("script written to file", "file", output) |
There was a problem hiding this comment.
P2: Misleading user output: success message is logged before deferred file.Close(), so if close fails, users see success message but program exits with error code 1. Consider moving the success log after explicitly closing the file, or handle the close error before logging success.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/cli/cli.go, line 248:
<comment>Misleading user output: success message is logged before deferred `file.Close()`, so if close fails, users see success message but program exits with error code 1. Consider moving the success log after explicitly closing the file, or handle the close error before logging success.</comment>
<file context>
@@ -119,3 +122,330 @@ func RunLoad(dir string, showSecrets bool) {
+ logger.Error("failed to write script to file", "error", err)
+ os.Exit(1)
+ }
+ logger.Info("script written to file", "file", output)
+ } else {
+ // Print to stdout
</file context>
| timelock_after: | ||
| type: integer |
There was a problem hiding this comment.
P1: Missing format: int64 for timelock_after field. This field represents a Cardano slot number which can exceed 32-bit integer limits. The slot field in ScriptValidateRequest correctly uses format: int64, but this field generates as int32, causing potential overflow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/swagger.yaml, line 41:
<comment>Missing `format: int64` for `timelock_after` field. This field represents a Cardano slot number which can exceed 32-bit integer limits. The `slot` field in `ScriptValidateRequest` correctly uses `format: int64`, but this field generates as `int32`, causing potential overflow.</comment>
<file context>
@@ -1,5 +1,105 @@
+ required:
+ minimum: 1
+ type: integer
+ timelock_after:
+ type: integer
+ timelock_before:
</file context>
| timelock_after: | |
| type: integer | |
| timelock_after: | |
| format: int64 | |
| type: integer |
| timelock_before: | ||
| type: integer |
There was a problem hiding this comment.
P1: Missing format: int64 for timelock_before field. Like timelock_after, this represents a Cardano slot number and should use 64-bit integers to avoid overflow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/swagger.yaml, line 43:
<comment>Missing `format: int64` for `timelock_before` field. Like `timelock_after`, this represents a Cardano slot number and should use 64-bit integers to avoid overflow.</comment>
<file context>
@@ -1,5 +1,105 @@
+ type: integer
+ timelock_after:
+ type: integer
+ timelock_before:
+ type: integer
+ type:
</file context>
| timelock_before: | |
| type: integer | |
| timelock_before: | |
| format: int64 | |
| type: integer |
| api.WalletRestoreRequest: | ||
| properties: | ||
| account_id: | ||
| maximum: 2147483647 |
There was a problem hiding this comment.
P2: Missing minimum: 0 constraint for derivation index fields. These BIP-32 derivation path indices must be non-negative (0 to 2147483647). Without this validation, the API would accept invalid negative values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At openapi/api/openapi.yaml, line 405:
<comment>Missing `minimum: 0` constraint for derivation index fields. These BIP-32 derivation path indices must be non-negative (0 to 2147483647). Without this validation, the API would accept invalid negative values.</comment>
<file context>
@@ -187,31 +402,42 @@ components:
api.WalletRestoreRequest:
properties:
account_id:
+ maximum: 2147483647
type: integer
address_id:
</file context>
| maximum: 2147483647 | |
| maximum: 2147483647 | |
| minimum: 0 |
wolf31o2
force-pushed
the
feat/cip-1854-multisig-support
branch
from
22f1fe2 to
846aa04
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openapi/api/openapi.yaml (1)
16-381: /api/script/create spec vs implementation mismatch (scriptHashvshash).The schema for
api.ScriptResponseand the/api/script/create200 response documents ascriptHashproperty, but the handler currently serializesbursa.ScriptDatawith ahashfield instead. This breaks generated clients that rely on the documentedscriptHash.Either:
- Change the handler to return
ScriptResponsewithscriptHashpopulated (preferred, see suggested diff ininternal/api/api.gocomment), or- Update this schema and related docs to use
hashconsistently, then regenerate the OpenAPI models/clients.Until they’re aligned, the API contract is misleading.
♻️ Duplicate comments (5)
internal/api/api.go (3)
529-549: WalletRestore error JSON is now safely encoded.Wrapping
err.Error()in amap[string]stringand marshaling viaencoding/jsonremoves the prior JSON injection risk while still returning useful messages to clients.
1006-1024: Timelock mutual-exclusion behavior is explicit and client-visible.Rejecting requests that specify both
timelock_beforeandtimelock_afterwith a clear 400 error avoids the earlier “silent ignore” behavior and is reasonable for v1 of the API.
143-150:ScriptValidateRequest.Networkis required but unused.The
Networkfield is validated and required but never referenced inhandleScriptValidate. This forces clients to supply a value that has no effect; consider either using it (if you plan network-specific validation) or relaxing/removing therequiredconstraint.openapi/model_api_script_validate_request.go (1)
23-303: ApiScriptValidateRequest model matches the updated spec and handler usage.Required/optional fields,
Slotas*int64, strictUnmarshalJSON, and the nullable wrapper all look correct for clients targeting/api/script/validate.openapi/api/openapi.yaml (1)
405-434: Derivation index fields still lackminimum: 0.
account_id,address_id,committee_cold_id,committee_hot_id,drep_id,payment_id, andstake_idnow havemaximum: 2147483647but nominimum. For BIP-32-style indices these should be constrained to0..2147483647; addingminimum: 0would prevent negative values from reaching the server.
🧹 Nitpick comments (6)
docs/swagger.yaml (1)
28-32: Consider adding maxItems constraint to key_hashes array.The
key_hashesarray hasminItems: 1but no upper bound. Without a maximum limit, clients could send arbitrarily large arrays, potentially causing memory exhaustion or degraded performance.Apply this diff to add a reasonable maximum:
key_hashes: items: type: string minItems: 1 + maxItems: 100 type: arrayAdjust the maxItems value based on your expected use case and system constraints.
internal/cli/cli.go (1)
260-296: Reduce duplication in scriptRequiresSignatures.The All, Any, and NofK cases duplicate the same script-conversion and recursion pattern. Consider extracting this logic:
+func checkNestedScripts(scripts []bursa.Script) bool { + wrappedScripts := make([]bursa.Script, len(scripts)) + for i, scr := range scripts { + wrappedScripts[i] = scr + } + return slices.ContainsFunc(wrappedScripts, scriptRequiresSignatures) +} + func scriptRequiresSignatures(script bursa.Script) bool { nativeScript, ok := script.(*bursa.NativeScript) if !ok { return false } switch s := nativeScript.Item().(type) { case *bursa.NativeScriptPubkey: return true case *bursa.NativeScriptAll: - scripts := make([]bursa.Script, len(s.Scripts)) - for i, scr := range s.Scripts { - scripts[i] = scr - } - if slices.ContainsFunc(scripts, scriptRequiresSignatures) { - return true - } + return checkNestedScripts(s.Scripts) case *bursa.NativeScriptAny: - scripts := make([]bursa.Script, len(s.Scripts)) - for i, scr := range s.Scripts { - scripts[i] = scr - } - if slices.ContainsFunc(scripts, scriptRequiresSignatures) { - return true - } + return checkNestedScripts(s.Scripts) case *bursa.NativeScriptNofK: - scripts := make([]bursa.Script, len(s.Scripts)) - for i, scr := range s.Scripts { - scripts[i] = scr - } - if slices.ContainsFunc(scripts, scriptRequiresSignatures) { - return true - } + return checkNestedScripts(s.Scripts) case *bursa.NativeScriptInvalidBefore, *bursa.NativeScriptInvalidHereafter: return false } return false }internal/api/api.go (2)
1048-1161: Script validation handler is structurally sound but ignores network.Unmarshaling via
bursa.UnmarshalScript, hex-decoding signatures, and delegating tobursa.ValidateScriptlooks correct, and errors are mapped to JSON safely. However, as noted earlier,req.Networkis never used; if network is irrelevant here, you may want to drop it from the request model to reduce noise.
1163-1260: Address generation handler behavior and error handling look good overall.
handleScriptAddresscleanly validates input, unmarshals the script, callsGetScriptAddressandGetScriptHash, and returns a well-shapedScriptAddressResponse. Errors are JSON-encoded via maps, avoiding string interpolation issues. If you want to refine status codes further, you could distinguishErrInvalidNetwork(400) from unexpected internal failures (500) whenGetScriptAddressreturns an error.openapi/api/openapi.yaml (1)
262-381: Scriptscriptproperty may be overspecified asadditionalProperties: { type: object }.For
api.ScriptAddressRequest,api.ScriptResponse, andapi.ScriptValidateRequest,scriptis modeled as an object whose values must themselves be JSON objects. If your actual script JSON (fromMarshalScript/UnmarshalScript) allows scalars or arrays at some positions, you may want to relax this to plainadditionalProperties: {}(or a looser schema) and regenerate clients, to avoid type mismatches.openapi/docs/BursaWallet.md (1)
7-15: New committee/DRep key fields are documented consistentlyThe additional
Committee*andDrep*key fields are correctly listed as optional pointers toBursaKeyFile, matching the underlying model and existing payment/stake key documentation style.If you want to improve consumer understanding, consider adding short descriptions for each of these fields in the OpenAPI spec so that this table includes more than empty description cells.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
bursa.go(5 hunks)bursa_test.go(2 hunks)cmd/bursa/main.go(1 hunks)cmd/bursa/script.go(1 hunks)docs/docs.go(3 hunks)docs/swagger.json(3 hunks)docs/swagger.yaml(2 hunks)internal/api/api.go(5 hunks)internal/api/api_test.go(1 hunks)internal/cli/cli.go(2 hunks)openapi.sh(1 hunks)openapi/.openapi-generator/FILES(2 hunks)openapi/README.md(2 hunks)openapi/api/openapi.yaml(6 hunks)openapi/api_default.go(14 hunks)openapi/docs/ApiScriptAddressRequest.md(1 hunks)openapi/docs/ApiScriptAddressResponse.md(1 hunks)openapi/docs/ApiScriptCreateRequest.md(1 hunks)openapi/docs/ApiScriptResponse.md(1 hunks)openapi/docs/ApiScriptValidateRequest.md(1 hunks)openapi/docs/ApiScriptValidateResponse.md(1 hunks)openapi/docs/BursaWallet.md(2 hunks)openapi/docs/DefaultAPI.md(2 hunks)openapi/model_api_script_address_request.go(1 hunks)openapi/model_api_script_address_response.go(1 hunks)openapi/model_api_script_create_request.go(1 hunks)openapi/model_api_script_response.go(1 hunks)openapi/model_api_script_validate_request.go(1 hunks)openapi/model_api_script_validate_response.go(1 hunks)openapi/model_bursa_wallet.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/bursa/script.go
- openapi/docs/ApiScriptAddressResponse.md
- openapi/model_api_script_address_response.go
- openapi/README.md
🧰 Additional context used
🧬 Code graph analysis (9)
bursa_test.go (1)
bursa.go (21)
NewScriptSig(858-872)NewScriptAll(875-897)NewScriptAny(900-922)NewScriptNOf(925-951)NewScriptBefore(954-968)NewScriptAfter(971-985)GetScriptHash(1087-1099)GetScriptAddress(1102-1124)ErrInvalidNetwork(47-47)NewMultiSigScript(989-1014)NativeScriptNofK(77-77)NewAllMultiSigScript(1017-1032)NativeScriptAll(75-75)NewAnyMultiSigScript(1035-1050)NewTimelockedScript(1053-1064)NativeScript(73-73)NewMultiSigScriptFromKeys(1068-1084)MarshalScript(1531-1559)UnmarshalScript(1562-1567)ScriptData(1523-1528)Script(69-69)
openapi/model_api_script_validate_response.go (1)
openapi/utils.go (2)
MappedNullable(354-356)IsNil(335-352)
openapi/model_api_script_address_request.go (2)
openapi/utils.go (1)
MappedNullable(354-356)bursa.go (1)
Script(69-69)
internal/api/api.go (2)
bursa.go (10)
Script(69-69)NewMultiSigScript(989-1014)NewAllMultiSigScript(1017-1032)NewAnyMultiSigScript(1035-1050)NewTimelockedScript(1053-1064)MarshalScript(1531-1559)UnmarshalScript(1562-1567)ScriptData(1523-1528)GetScriptHash(1087-1099)GetScriptAddress(1102-1124)internal/logging/logging.go (1)
GetLogger(78-83)
openapi/model_api_script_response.go (1)
openapi/utils.go (2)
MappedNullable(354-356)IsNil(335-352)
openapi/model_api_script_create_request.go (1)
openapi/utils.go (2)
MappedNullable(354-356)IsNil(335-352)
openapi/model_api_script_validate_request.go (2)
openapi/utils.go (2)
MappedNullable(354-356)IsNil(335-352)bursa.go (1)
Script(69-69)
openapi/model_bursa_wallet.go (2)
openapi/model_bursa_key_file.go (1)
BursaKeyFile(22-26)openapi/utils.go (1)
IsNil(335-352)
openapi/api_default.go (7)
openapi/model_api_script_address_response.go (1)
ApiScriptAddressResponse(22-26)openapi/model_api_script_response.go (1)
ApiScriptResponse(22-27)openapi/model_api_script_validate_response.go (1)
ApiScriptValidateResponse(22-27)openapi/model_api_script_address_request.go (1)
ApiScriptAddressRequest(24-27)openapi/client.go (1)
GenericOpenAPIError(655-659)openapi/model_api_script_create_request.go (1)
ApiScriptCreateRequest(24-31)openapi/model_api_script_validate_request.go (1)
ApiScriptValidateRequest(24-30)
🪛 Checkov (3.2.334)
docs/swagger.yaml
[medium] 29-34: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 LanguageTool
openapi/docs/ApiScriptValidateRequest.md
[grammar] ~52-~52: Ensure spelling is correct
Context: ...ets Network field to given value. ### GetRequireSignatures func (o *ApiScriptValidateRequest) GetRequireSignatures() bool GetRequireSignatures returns the Require...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ... if non-nil, zero value otherwise. ### GetRequireSignaturesOk func (o *ApiScriptValidateRequest) GetRequireSignaturesOk() (*bool, bool) GetRequireSignaturesOk returns a tuple w...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
openapi/docs/ApiScriptCreateRequest.md
7-7: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
7-7: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
8-8: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
8-8: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
12-12: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
12-12: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
openapi/docs/ApiScriptValidateRequest.md
7-7: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
7-7: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
9-9: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
9-9: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
openapi/docs/DefaultAPI.md
33-33: Hard tabs
Column: 1
(MD010, no-hard-tabs)
34-34: Hard tabs
Column: 1
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 1
(MD010, no-hard-tabs)
36-36: Hard tabs
Column: 1
(MD010, no-hard-tabs)
40-40: Hard tabs
Column: 1
(MD010, no-hard-tabs)
42-42: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1
(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
65-65: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
65-65: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
80-80: No empty links
(MD042, no-empty-links)
99-99: Hard tabs
Column: 1
(MD010, no-hard-tabs)
100-100: Hard tabs
Column: 1
(MD010, no-hard-tabs)
101-101: Hard tabs
Column: 1
(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 1
(MD010, no-hard-tabs)
106-106: Hard tabs
Column: 1
(MD010, no-hard-tabs)
108-108: Hard tabs
Column: 1
(MD010, no-hard-tabs)
109-109: Hard tabs
Column: 1
(MD010, no-hard-tabs)
110-110: Hard tabs
Column: 1
(MD010, no-hard-tabs)
111-111: Hard tabs
Column: 1
(MD010, no-hard-tabs)
112-112: Hard tabs
Column: 1
(MD010, no-hard-tabs)
113-113: Hard tabs
Column: 1
(MD010, no-hard-tabs)
114-114: Hard tabs
Column: 1
(MD010, no-hard-tabs)
115-115: Hard tabs
Column: 1
(MD010, no-hard-tabs)
116-116: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
131-131: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
146-146: No empty links
(MD042, no-empty-links)
165-165: Hard tabs
Column: 1
(MD010, no-hard-tabs)
166-166: Hard tabs
Column: 1
(MD010, no-hard-tabs)
167-167: Hard tabs
Column: 1
(MD010, no-hard-tabs)
168-168: Hard tabs
Column: 1
(MD010, no-hard-tabs)
172-172: Hard tabs
Column: 1
(MD010, no-hard-tabs)
174-174: Hard tabs
Column: 1
(MD010, no-hard-tabs)
175-175: Hard tabs
Column: 1
(MD010, no-hard-tabs)
176-176: Hard tabs
Column: 1
(MD010, no-hard-tabs)
177-177: Hard tabs
Column: 1
(MD010, no-hard-tabs)
178-178: Hard tabs
Column: 1
(MD010, no-hard-tabs)
179-179: Hard tabs
Column: 1
(MD010, no-hard-tabs)
180-180: Hard tabs
Column: 1
(MD010, no-hard-tabs)
181-181: Hard tabs
Column: 1
(MD010, no-hard-tabs)
182-182: Hard tabs
Column: 1
(MD010, no-hard-tabs)
197-197: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
197-197: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
212-212: No empty links
(MD042, no-empty-links)
⏰ 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). (5)
- GitHub Check: nilaway
- GitHub Check: docker
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (31)
openapi.sh (1)
5-5: LGTM! Portable sed implementation.The OS detection properly handles the
sed -iportability issue between macOS and Linux that was flagged in previous reviews.bursa.go (1)
1126-1156: Validation implementation is well-documented as structural-only.The
ValidateScriptfunction and supporting documentation clearly indicate this is structural/format validation only, not cryptographic verification. The comments (lines 1158-1166) appropriately warn about the placeholder nature and include a TODO for Ed25519 verification.This addresses previous concerns about misleading documentation. The implementation is appropriate for testing script structure logic.
bursa_test.go (1)
882-1406: LGTM! Comprehensive test coverage for script functionality.The test suite thoroughly exercises:
- Script type construction and validation
- Hash and address generation
- Multi-sig key derivation (CIP-1854)
- Validation with various signature scenarios
- Edge cases and error conditions
- Round-trip serialization
The use of
ed25519.GenerateKey(nil)for random keys in tests (line 1122) is appropriate for these functional tests.cmd/bursa/main.go (1)
42-42: LGTM! Clean CLI integration.The
scriptCommand()addition follows the established pattern for CLI subcommands.openapi/docs/ApiScriptCreateRequest.md (1)
1-171: Auto-generated API documentation looks correct.This documents the new
ApiScriptCreateRequestmodel with appropriate properties and methods. The markdown formatting warnings from linters are cosmetic issues in generated code.openapi/docs/ApiScriptValidateResponse.md (1)
1-134: Auto-generated API documentation looks correct.This documents the new
ApiScriptValidateResponsemodel with appropriate optional fields and accessors.openapi/docs/ApiScriptValidateRequest.md (1)
1-150: Auto-generated API documentation looks correct.This documents the new
ApiScriptValidateRequestmodel with the script, signatures, slot, and requireSignatures fields appropriately documented.openapi/docs/DefaultAPI.md (1)
7-215: New API endpoints properly documented.The three new script endpoints (
ApiScriptAddressPost,ApiScriptCreatePost,ApiScriptValidatePost) are well-documented with examples, parameters, return types, and proper API structure.internal/api/api_test.go (1)
658-855: LGTM! Comprehensive test coverage for script endpoints.The test suite covers both happy paths and error cases well:
TestHandleScriptCreatevalidates successful script creationTestHandleScriptValidateverifies validation logic including the criticalvalidfieldTestHandleScriptValidateErrorsuses table-driven tests for multiple error scenariosTestHandleScriptAddresschecks address generationThe tests appropriately verify response status codes and JSON field presence.
internal/cli/cli.go (3)
126-258: LGTM! Validation and error handling are robust.The function properly validates:
- Mutually exclusive flags (--all, --any, --required)
- Timelock conflicts (can't specify both before and after)
- Required count doesn't exceed key hashes
- Hex decoding of key hashes
File close errors are now handled explicitly, addressing previous concerns.
298-385: LGTM! Script validation logic is sound.The function correctly:
- Unmarshals the script from file
- Recomputes the script hash (addressing previous concerns about trusting file contents)
- Validates signature requirements
- Returns comprehensive validation results
387-451: LGTM! Address generation is correctly implemented.The function properly:
- Unmarshals the script
- Recomputes the script hash for accuracy
- Generates the network-appropriate address
openapi/.openapi-generator/FILES (1)
1-37: Auto-generated file listing.This file tracks OpenAPI generator artifacts and requires no manual review.
openapi/docs/ApiScriptAddressRequest.md (1)
1-72: Auto-generated API documentation.This documentation file is generated by OpenAPI Generator and requires no manual review.
docs/swagger.json (1)
22-508: LGTM! Script API definitions are well-structured.The new script endpoints and definitions are properly specified:
- Consistent field naming (scriptHash across all responses)
- Appropriate data types (int64 for slot/timelock fields)
- Standard request/response patterns
- Proper validation constraints (minItems, enums)
Previous concerns about type formats and naming consistency have been addressed.
openapi/model_api_script_create_request.go (1)
1-330: LGTM! Well-generated OpenAPI model.The generated code correctly implements:
- Proper type mappings (int32 for Required, int64 for timelock fields)
- Comprehensive accessor methods with Has* checks for optional fields
- Strict JSON unmarshaling with
DisallowUnknownFields()- Nullable wrapper pattern for optional usage
openapi/model_api_script_validate_response.go (1)
1-237: LGTM! Properly generated response model.The generated code correctly implements:
- Pointer fields with omitempty for optional response data
- Full accessor suite with Has* methods
- Appropriate type mappings (int32 for Signatures, int64 for Slot)
- Nullable wrapper for flexible usage
openapi/model_api_script_address_request.go (1)
23-157: ApiScriptAddressRequest model looks consistent and robust.Required-field checks plus
DisallowUnknownFieldsand the nullable wrapper are all in line with the rest of the generated models; no issues from a client/SDK perspective.internal/api/api.go (3)
133-141: ScriptCreateRequest validation and shape look coherent.Type/Required/KeyHashes/Network validation matches the documented script-creation semantics and should guard most bad inputs before business logic.
152-179: ScriptAddressRequest/Response structs align with handler behavior.
ScriptAddressRequestmatches the handler’s expectations, andScriptAddressResponse(Address/Network/ScriptHash) matches whathandleScriptAddressactually serializes.
258-262: Script routes are correctly registered on the main mux.New
/api/script/create,/api/script/validate, and/api/script/addresshandlers are wired intoStartalongside existing wallet routes; no issues here.docs/docs.go (1)
26-512: Swagger docs correctly describe the new script endpoints.The added
/api/script/*paths and associatedapi.Script*definitions line up with the new handlers and OpenAPI schemas; nothing problematic from a documentation perspective.openapi/docs/ApiScriptResponse.md (1)
1-134: ApiScriptResponse docs are in sync with the model.The documented fields and helper methods match the generated
ApiScriptResponsetype and its JSON shape; no changes needed.openapi/model_api_script_response.go (1)
21-235: ApiScriptResponse implementation is consistent with other generated models.Getters/setters,
ToMap, customMarshalJSON, and the nullable wrapper all follow the standard pattern and correctly honor optional fields.openapi/model_bursa_wallet.go (1)
21-701: BursaWallet extensions for committee/DRep keys look correct.New committee_* and drep_* key fields, their accessors, and
ToMapwiring are consistent with existing payment/stake fields and the updated OpenAPI schema.openapi/api_default.go (5)
24-41: New script methods correctly extend DefaultAPI interfaceThe three new Script* methods on
DefaultAPI(ScriptAddressPost, ScriptCreatePost, ScriptValidatePost) follow the same builder/execute pattern as the existing wallet methods and expose appropriate response types. The context plumbing and return signatures look consistent with the rest of the interface.
184-349: ApiScriptAddressPost client implementation looks correct and consistent
- Uses
POSTto/api/script/addresswithapplication/jsonbody, which aligns with the newApiScriptAddressRequestmodel.- Validates that
r.requestis non‑nil before issuing the call, returning a clearreportErrorotherwise.- Error handling for 400/500 and success decoding into
*ApiScriptAddressResponsematches existing wallet endpoint patterns, including buffering and restoring the response body.No issues from a correctness or client‑side behavior perspective.
351-516: ApiScriptCreatePost client implementation is structurally sound
- Correctly targets
POST /api/script/createusing JSON and enforces a non‑nilApiScriptCreateRequest.- Error and decode paths mirror
ApiWallet*methods, including special handling for HTTP 400 and 500 into astringmodel.- The
Requestbuilder andExecutehelper onDefaultAPIApiScriptCreatePostRequestfollow the same value‑receiver pattern used elsewhere in this file.Looks good and aligned with the generated client conventions.
518-683: ApiScriptValidatePost client implementation is correct and in line with existing endpoints
- Uses
POST /api/script/validatewith theApiScriptValidateRequestas JSON body and validatesr.request != nil.- HTTP method, header selection (
Content-Type/Accept=application/json), and error decoding for 400/500 follow the exact pattern of other generated methods.- Decodes into
*ApiScriptValidateResponseand preserves the raw body viaio.NopCloser, consistent with the rest of the client.No behavioral or structural issues spotted.
88-90: Doc comment tweaks for@returntags are benignThe small formatting changes to
// @return ...comments on existing wallet methods are purely cosmetic and keep documentation consistent; they don’t affect behavior.Also applies to: 106-108, 122-124, 138-140, 156-158, 174-176, 711-715, 842-845, 1009-1012, 1167-1170, 1298-1301, 1465-1468
openapi/docs/BursaWallet.md (1)
45-268: Accessor documentation for new key fields aligns with existing BursaWallet patternsFor each new committee/DRep key field, the generated
Get*,Get*Ok,Set*, andHas*method docs follow the exact same structure and semantics as the existing payment/stake key accessors. This keeps the public API predictable for consumers working with the richer wallet model.No issues from a documentation/API‑surface standpoint.
| // NewScriptAll creates an "all" script requiring all sub-scripts to be satisfied | ||
| func NewScriptAll(scripts ...Script) *NativeScript { | ||
| nativeScripts := make([]NativeScript, len(scripts)) | ||
| for i, script := range scripts { | ||
| nativeScript, ok := script.(*NativeScript) | ||
| if !ok { | ||
| return nil | ||
| } | ||
| nativeScripts[i] = *nativeScript | ||
| } | ||
| concrete := &NativeScriptAll{ | ||
| Type: 1, | ||
| Scripts: nativeScripts, | ||
| } | ||
| cborData, err := cbor.Encode(concrete) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| var script NativeScript | ||
| if _, err := cbor.Decode(cborData, &script); err != nil { | ||
| return nil | ||
| } | ||
| return &script | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent error handling: CBOR encoding errors are silently ignored.
NewScriptAll (and similarly NewScriptAny, NewScriptNOf, NewScriptBefore, NewScriptAfter) performs CBOR encoding that can fail, but returns nil without an error value. This creates two issues:
- Callers cannot distinguish between success-with-nil-result and encoding-failure
- Higher-level functions like
NewMultiSigScript(line 1013) return(nil, nil)when these functions fail, masking the error
Consider either:
- Option 1: Change these functions to return
(*NativeScript, error)for consistency withNewScriptSig - Option 2: Make these functions panic on CBOR encoding errors (since encoding failure represents a programming error with the internal structures)
Apply this diff for Option 1 (NewScriptAll example; repeat for other functions):
-func NewScriptAll(scripts ...Script) *NativeScript {
+func NewScriptAll(scripts ...Script) (*NativeScript, error) {
nativeScripts := make([]NativeScript, len(scripts))
for i, script := range scripts {
nativeScript, ok := script.(*NativeScript)
if !ok {
- return nil
+ return nil, errors.New("script must be a *NativeScript")
}
nativeScripts[i] = *nativeScript
}
concrete := &NativeScriptAll{
Type: 1,
Scripts: nativeScripts,
}
cborData, err := cbor.Encode(concrete)
if err != nil {
- return nil
+ return nil, err
}
var script NativeScript
if _, err := cbor.Decode(cborData, &script); err != nil {
- return nil
+ return nil, err
}
- return &script
+ return &script, nil
}Then update all callers to handle the returned error.
Committable suggestion skipped: line range outside the PR's diff.
| scripts := make([]Script, len(keyHashes)) | ||
| for i, keyHash := range keyHashes { | ||
| script, err := NewScriptSig(keyHash) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| scripts[i] = script | ||
| } | ||
|
|
||
| return NewScriptNOf(required, scripts...), nil |
There was a problem hiding this comment.
Potential (nil, nil) return on CBOR encoding failure.
When NewScriptSig succeeds but NewScriptNOf returns nil due to a CBOR encoding error, this function returns (nil, nil), which is ambiguous for callers.
This is a consequence of the inconsistent error handling in the script constructor functions. Once those functions are updated to return errors (per the previous comment), update this code to propagate the error:
scripts := make([]Script, len(keyHashes))
for i, keyHash := range keyHashes {
script, err := NewScriptSig(keyHash)
if err != nil {
return nil, err
}
scripts[i] = script
}
- return NewScriptNOf(required, scripts...), nil
+ result, err := NewScriptNOf(required, scripts...)
+ if err != nil {
+ return nil, fmt.Errorf("failed to create N-of script: %w", err)
+ }
+ return result, nilCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In bursa.go around lines 1004 to 1013, the call to NewScriptNOf assumes it
always returns a valid Script and can lead to an ambiguous (nil, nil) when CBOR
encoding fails; change the code to capture both return values (script, err :=
NewScriptNOf(required, scripts...)), check err and return nil, err on error, and
also guard against a nil script by returning a descriptive error if script ==
nil but err == nil so callers never receive (nil, nil).
Signed-off-by: Chris Gianelloni <[email protected]>
wolf31o2
force-pushed
the
feat/cip-1854-multisig-support
branch
from
846aa04 to
37830d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/api/api.go (1)
133-141: FixRequiredfield validation: current tag still rejects validall/anyscripts.The validation tag on
Requiredis effectively still:
required_if=Type nOf(correct), andmin=1unconditionally (becauseomitemptyis glued into the param string, not a separate rule).For
Type="all"or"any", clients typically omitrequired; it deserializes as 0 andmin=1will fail, so valid requests are rejected unless they set a meaninglessrequiredvalue.You want
omitemptyas its own rule so thatmin=1is skipped when the field is zero for non‑nOftypes. For example:type ScriptCreateRequest struct { - Type string `json:"type" validate:"required,oneof=nOf all any"` - Required int `json:"required" validate:"required_if=Type nOf,omitempty,min=1"` + Type string `json:"type" validate:"required,oneof=nOf all any"` + Required int `json:"required" validate:"omitempty,min=1,required_if=Type nOf"` KeyHashes []string `json:"key_hashes" validate:"required,min=1,dive,hexadecimal,len=56"` TimelockBefore uint64 `json:"timelock_before,omitempty" swaggertype:"integer" format:"int64"` TimelockAfter uint64 `json:"timelock_after,omitempty" swaggertype:"integer" format:"int64"` Network string `json:"network" validate:"required,oneof=mainnet testnet"` }This ensures:
- For
nOf,Requiredmust be present and ≥1.- For
all/any,Requiredcan be omitted and won’t triggermin=1.
🧹 Nitpick comments (5)
docs/swagger.yaml (1)
28-32: Consider addingmaxItemsconstraint tokey_hashesarray.The
key_hashesarray hasminItems: 1but no upper bound. While the static analysis hint (CKV_OPENAPI_21) flags this, it's worth considering whether an unbounded array could pose a resource concern. A practical upper limit (e.g., 100 or 1000) would prevent potential abuse while still supporting legitimate use cases.key_hashes: items: type: string minItems: 1 + maxItems: 100 type: arraybursa_test.go (1)
957-979: Use realistic 28-byte key hashes in these tests.Both
TestGetScriptHashandTestGetScriptAddresscreate scripts with 3-byte key hashes ([]byte{0x01, 0x02, 0x03}), but Cardano uses Blake2b-224 hashes which are 28 bytes. WhileNewScriptSigdoesn't currently validate hash length, using realistic 28-byte key hashes would make tests more representative of actual usage and better guard against future validation additions. SeeNewMultiSigScriptFromKeys(line 1066+) for how real key hashes are created.openapi/docs/ApiScriptAddressRequest.md (1)
5-8: Fix markdown table to satisfy markdownlint (pipe style / column count).markdownlint is flagging the properties table for
ApiScriptAddressRequest(unexpected leading/trailing pipes and column count mismatch). You can normalize the table to 4 columns without leading/trailing pipes to clear those warnings. For example:-Name | Type | Description | Notes | ------------- | ------------- | ------------- | ------------- | -**Network** | **string** | | | -**Script** | **map[string]map[string]interface{}** | | | +Name | Type | Description | Notes +------------ | ------------- | ------------- | ------------- +**Network** | **string** | | +**Script** | **map[string]map[string]interface{}** | |internal/api/api.go (1)
1056-1169: handleScriptValidate implements the expected validation semantics;Networkis currently informational.
- The handler validates the structure of
script, optionalnetwork(mainnet|testnet),signaturesas 128‑char hex, and optionalslot.- Script parsing via
bursa.UnmarshalScriptand validation viabursa.ValidateScriptplusGetScriptHashproduceScriptValidateResponsewithscriptHash, signature count, slot, and validity, matching the public schema.Networkis accepted/validated but not used in the logic yet; that’s fine if it’s intentionally reserved for future network-specific checks, but if you don’t plan to use it, you might consider documenting it as informational or trimming it from the request to avoid confusion.Overall the flow looks correct.
openapi/api/openapi.yaml (1)
290-321: Consider adding maximum constraint forrequiredfield.The
requiredfield hasminimum: 1but no maximum constraint. For nOf scripts,requiredshould not exceed the number ofkey_hashes. Consider adding a schema-level constraint or documenting that backend validation enforcesrequired ≤ len(key_hashes).required: minimum: 1 + # Note: Must not exceed the length of key_hashes array type: integer
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
bursa.go(5 hunks)bursa_test.go(2 hunks)cmd/bursa/main.go(1 hunks)cmd/bursa/script.go(1 hunks)docs/docs.go(3 hunks)docs/swagger.json(3 hunks)docs/swagger.yaml(2 hunks)internal/api/api.go(5 hunks)internal/api/api_test.go(1 hunks)internal/cli/cli.go(2 hunks)openapi.sh(1 hunks)openapi/.openapi-generator/FILES(2 hunks)openapi/README.md(2 hunks)openapi/api/openapi.yaml(6 hunks)openapi/api_default.go(14 hunks)openapi/docs/ApiScriptAddressRequest.md(1 hunks)openapi/docs/ApiScriptAddressResponse.md(1 hunks)openapi/docs/ApiScriptCreateRequest.md(1 hunks)openapi/docs/ApiScriptResponse.md(1 hunks)openapi/docs/ApiScriptValidateRequest.md(1 hunks)openapi/docs/ApiScriptValidateResponse.md(1 hunks)openapi/docs/BursaWallet.md(2 hunks)openapi/docs/DefaultAPI.md(2 hunks)openapi/model_api_script_address_request.go(1 hunks)openapi/model_api_script_address_response.go(1 hunks)openapi/model_api_script_create_request.go(1 hunks)openapi/model_api_script_response.go(1 hunks)openapi/model_api_script_validate_request.go(1 hunks)openapi/model_api_script_validate_response.go(1 hunks)openapi/model_bursa_wallet.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- openapi/docs/ApiScriptValidateResponse.md
🚧 Files skipped from review as they are similar to previous changes (5)
- openapi.sh
- openapi/model_api_script_address_response.go
- cmd/bursa/main.go
- openapi/docs/ApiScriptResponse.md
- openapi/api_default.go
🧰 Additional context used
🧬 Code graph analysis (7)
bursa_test.go (1)
bursa.go (31)
NewScriptSig(858-872)NewScriptAll(875-897)NewScriptAny(900-922)NewScriptNOf(925-951)NewScriptBefore(954-968)NewScriptAfter(971-985)GetScriptHash(1087-1099)GetScriptAddress(1102-1124)ErrInvalidNetwork(47-47)GetRootKeyFromMnemonic(431-445)GetMultiSigAccountKey(763-773)GetMultiSigPaymentKey(776-784)GetMultiSigPaymentVKey(811-817)GetMultiSigPaymentSKey(820-826)GetMultiSigStakeKey(829-837)GetMultiSigStakeVKey(840-846)GetMultiSigStakeSKey(849-855)ValidateScript(1129-1156)NewMultiSigScript(989-1014)NativeScriptNofK(77-77)NewAllMultiSigScript(1017-1032)NativeScriptAll(75-75)NewAnyMultiSigScript(1035-1050)NativeScriptAny(76-76)NewTimelockedScript(1053-1064)NativeScript(73-73)NewMultiSigScriptFromKeys(1068-1084)MarshalScript(1531-1559)UnmarshalScript(1562-1567)ScriptData(1523-1528)Script(69-69)
openapi/model_api_script_address_request.go (2)
openapi/utils.go (1)
MappedNullable(354-356)bursa.go (1)
Script(69-69)
internal/api/api.go (2)
bursa.go (10)
Script(69-69)NewMultiSigScript(989-1014)NewAllMultiSigScript(1017-1032)NewAnyMultiSigScript(1035-1050)NewTimelockedScript(1053-1064)MarshalScript(1531-1559)UnmarshalScript(1562-1567)ScriptData(1523-1528)GetScriptHash(1087-1099)GetScriptAddress(1102-1124)internal/logging/logging.go (1)
GetLogger(78-83)
openapi/model_api_script_create_request.go (1)
openapi/utils.go (2)
MappedNullable(354-356)IsNil(335-352)
openapi/model_api_script_validate_request.go (2)
openapi/utils.go (2)
MappedNullable(354-356)IsNil(335-352)bursa.go (1)
Script(69-69)
openapi/model_bursa_wallet.go (2)
openapi/model_bursa_key_file.go (1)
BursaKeyFile(22-26)openapi/utils.go (1)
IsNil(335-352)
bursa.go (1)
bip32/bip32.go (2)
XPrv(43-43)PublicKey(49-49)
🪛 Checkov (3.2.334)
docs/swagger.yaml
[medium] 29-34: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 LanguageTool
openapi/docs/ApiScriptValidateRequest.md
[grammar] ~57-~57: Ensure spelling is correct
Context: ...a boolean if a field has been set. ### GetRequireSignatures func (o *ApiScriptValidateRequest) GetRequireSignatures() bool GetRequireSignatures returns the Require...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~63-~63: Ensure spelling is correct
Context: ... if non-nil, zero value otherwise. ### GetRequireSignaturesOk func (o *ApiScriptValidateRequest) GetRequireSignaturesOk() (*bool, bool) GetRequireSignaturesOk returns a tuple w...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
openapi/docs/ApiScriptAddressRequest.md
7-7: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
7-7: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
8-8: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
8-8: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
openapi/README.md
95-95: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
96-96: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
97-97: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
98-98: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
99-99: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
100-100: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
openapi/docs/ApiScriptCreateRequest.md
7-7: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
7-7: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
8-8: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
8-8: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
12-12: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
12-12: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
openapi/docs/ApiScriptValidateRequest.md
9-9: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
9-9: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
openapi/docs/DefaultAPI.md
33-33: Hard tabs
Column: 1
(MD010, no-hard-tabs)
34-34: Hard tabs
Column: 1
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 1
(MD010, no-hard-tabs)
36-36: Hard tabs
Column: 1
(MD010, no-hard-tabs)
40-40: Hard tabs
Column: 1
(MD010, no-hard-tabs)
42-42: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1
(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
65-65: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
65-65: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
80-80: No empty links
(MD042, no-empty-links)
99-99: Hard tabs
Column: 1
(MD010, no-hard-tabs)
100-100: Hard tabs
Column: 1
(MD010, no-hard-tabs)
101-101: Hard tabs
Column: 1
(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 1
(MD010, no-hard-tabs)
106-106: Hard tabs
Column: 1
(MD010, no-hard-tabs)
108-108: Hard tabs
Column: 1
(MD010, no-hard-tabs)
109-109: Hard tabs
Column: 1
(MD010, no-hard-tabs)
110-110: Hard tabs
Column: 1
(MD010, no-hard-tabs)
111-111: Hard tabs
Column: 1
(MD010, no-hard-tabs)
112-112: Hard tabs
Column: 1
(MD010, no-hard-tabs)
113-113: Hard tabs
Column: 1
(MD010, no-hard-tabs)
114-114: Hard tabs
Column: 1
(MD010, no-hard-tabs)
115-115: Hard tabs
Column: 1
(MD010, no-hard-tabs)
116-116: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
131-131: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
146-146: No empty links
(MD042, no-empty-links)
165-165: Hard tabs
Column: 1
(MD010, no-hard-tabs)
166-166: Hard tabs
Column: 1
(MD010, no-hard-tabs)
167-167: Hard tabs
Column: 1
(MD010, no-hard-tabs)
168-168: Hard tabs
Column: 1
(MD010, no-hard-tabs)
172-172: Hard tabs
Column: 1
(MD010, no-hard-tabs)
174-174: Hard tabs
Column: 1
(MD010, no-hard-tabs)
175-175: Hard tabs
Column: 1
(MD010, no-hard-tabs)
176-176: Hard tabs
Column: 1
(MD010, no-hard-tabs)
177-177: Hard tabs
Column: 1
(MD010, no-hard-tabs)
178-178: Hard tabs
Column: 1
(MD010, no-hard-tabs)
179-179: Hard tabs
Column: 1
(MD010, no-hard-tabs)
180-180: Hard tabs
Column: 1
(MD010, no-hard-tabs)
181-181: Hard tabs
Column: 1
(MD010, no-hard-tabs)
182-182: Hard tabs
Column: 1
(MD010, no-hard-tabs)
197-197: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
197-197: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
212-212: No empty links
(MD042, no-empty-links)
⏰ 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). (3)
- GitHub Check: docker
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (48)
openapi/README.md (1)
82-100: Documentation correctly reflects the new Script API surface.The new API endpoints and model references are properly documented and align with the OpenAPI specification updates for the CIP-1854 multi-signature feature.
openapi/docs/ApiScriptAddressResponse.md (1)
1-107: Auto-generated model documentation is complete.The
ApiScriptAddressResponsemodel documentation correctly describes the response structure for the script address endpoint with all optional fields properly documented.openapi/.openapi-generator/FILES (1)
8-29: Generator manifest correctly updated with new script model files.The FILES manifest properly tracks all new script-related model and documentation files generated for the CIP-1854 feature.
bursa.go (7)
1052-1064:NewTimelockedScriptinherits the timelock naming confusion.The
beforeparameter's meaning is unclear due to the underlying timelock function naming issue. Once the timelock functions are corrected, this wrapper should be reviewed for clarity.
67-99: Script type aliases and GetScriptType helper are well-implemented.The type aliases provide clean integration with gouroboros types, and
GetScriptTypesafely handles edge cases (nil, non-NativeScript, empty CBOR).
762-855: CIP-1854 multi-sig key derivation helpers follow established patterns.The derivation path
1854'/1815'/account'correctly implements CIP-1854, and the helper functions (GetMultiSigPaymentVKey,GetMultiSigPaymentSKey, etc.) reuse existing key file creation patterns for consistency.
857-872:NewScriptSigproperly returns errors unlike other script constructors.This function correctly returns
(*NativeScript, error), providing clear error handling. The other script constructors (NewScriptAll,NewScriptAny,NewScriptNOf) returnnilon error without error information—a pattern noted in past reviews.
1086-1124: Script hashing and address generation are correctly implemented.
GetScriptHashuses Blake2b-224 as specified for Cardano script hashes, andGetScriptAddressproperly createsAddressTypeScriptNoneaddresses. Good null checks and error propagation.
1522-1638: ScriptData serialization is well-structured.
MarshalScriptandscriptToMaphandle all native script types with proper recursive serialization. The optional network parameter for address generation is a good API design choice.
1640-1770:mapToScriptdeserialization properly validates inputs.Good input validation including:
- Integer check for
n(line 1711-1713)- Range check for
n(line 1719)- Non-negative integer check for slots (lines 1748-1752, 1760-1763)
Previous review concerns about non-integral values have been addressed.
openapi/docs/ApiScriptCreateRequest.md (1)
1-171: Auto-generated model documentation is complete and accurate.The
ApiScriptCreateRequestdocumentation correctly describes the request structure for script creation with required fields (KeyHashes,Network,Type) and optional fields (Required,TimelockAfter,TimelockBefore). The static analysis warnings about table formatting are false positives—the format is consistent with OpenAPI generator output.openapi/docs/ApiScriptValidateRequest.md (1)
1-155: Auto-generated OpenAPI documentation looks correct.This file accurately documents the
ApiScriptValidateRequestmodel with its properties and accessor methods. The static analysis hints about table formatting and grammar are false positives typical of auto-generated OpenAPI docs.internal/api/api_test.go (4)
658-691: Good test coverage for script creation endpoint.The test validates the happy path for creating a 2-of-3 multi-sig script with proper assertions on response status and JSON field presence.
693-732: Script validation test properly verifies the response structure.Good practice to decode the JSON response and explicitly assert
valid == truerather than just checking for string presence.
734-818: Comprehensive error case coverage for script validation.The table-driven tests cover important error scenarios including invalid hex in keyHash, missing required fields, invalid network, and unsupported script types. The test case name "invalid keyHash format" on line 742 correctly describes the test intent.
820-855: Script address generation test validates mainnet address prefix.Good test that verifies the response contains expected fields and that the generated address starts with
addr1for mainnet.docs/swagger.yaml (1)
41-46: Timelock fields now correctly useint64format.The
timelock_afterandtimelock_beforefields now includeformat: int64, matching theslotfield inScriptValidateRequest. This ensures consistent handling of Cardano slot numbers that may exceed 32-bit limits.cmd/bursa/script.go (4)
22-34: Well-structured root script command with subcommands.The command hierarchy is clean with appropriate short descriptions.
36-97: Script create command implementation looks good.The flag definitions align with the CLI handler, and the help text provides clear usage examples. The variable
anyis renamed touseAnyto avoid shadowing Go's built-inanytype alias—good practice.
99-143: Script validate command correctly implements structural-only mode.The help text examples now accurately describe the
--structural-onlyflag for timelock-only validation, addressing the previous review feedback.
145-177: Script address command is straightforward.Clean implementation with required
--scriptflag and sensiblemainnetdefault for network.bursa_test.go (6)
18-18: Import added for Ed25519 key generation in tests.Needed for
TestMultiSigScriptFromKeysto generate test public keys.
882-955: Comprehensive script type tests with proper CBOR verification.Tests cover all script types (sig, all, any, nOf, before, after) and verify both constructor behavior and CBOR encoding output.
1080-1118: Good coverage of multi-sig script construction and timelocking.Tests verify correct script type identification and internal structure for 2-of-3, All, Any, and timelocked scripts.
1120-1136: Ed25519 key-based script creation test is well-designed.Uses
ed25519.GenerateKey(nil)withnilreader for deterministic test keys, then verifies the resulting script structure.
1292-1322: Timelock boundary tests correctly verify Cardano semantics.
ScriptBefore(1000)is correctly validated as false at slot 1000 (must be < threshold)ScriptAfter(1000)is correctly validated as true at slot 1000 (must be >= threshold)This aligns with Cardano's
RequireTimeAftersemantics.
1324-1406: Round-trip marshaling tests provide good coverage.Tests verify that scripts can be serialized and deserialized without data loss, and properly handle invalid input types.
openapi/docs/DefaultAPI.md (1)
1-215: New script API endpoints are properly documented.The auto-generated documentation correctly describes the three new endpoints (
/api/script/address,/api/script/create,/api/script/validate) with accurate request/response types and usage examples. The static analysis hints about tabs and table formatting are expected artifacts of the OpenAPI generator.internal/cli/cli.go (6)
126-161: Comprehensive parameter validation addresses previous review feedback.The validation logic correctly:
- Prevents mutually exclusive flags (
--alland--any, lines 136-139)- Prevents
--requiredwith--allor--any(lines 140-147)- Requires at least one flag type (lines 148-151)
- Validates at least one key hash (lines 152-155)
- Prevents both timelocks from being specified (lines 156-161)
180-190: Validation ensuresrequireddoesn't exceed key hash count.This addresses the previous review feedback about preventing invalid script construction.
221-258: File handling now properly checks close errors.The explicit
file.Close()with error check (lines 243-247) before logging success (line 248) addresses the previous review feedback about potentially misleading success messages and unchecked close errors.
260-295: Script signature requirement detection is well-implemented.The
scriptRequiresSignaturesfunction correctly traverses nested scripts to determine if any component requires signatures, withanyScriptRequiresSignaturesusing the idiomaticslices.ContainsFunchelper.
332-338: Script hash is now recomputed from the parsed script.This addresses the previous review feedback about trusting the hash from the input file. Computing a fresh hash ensures integrity even if the file was modified.
386-450: Script address generation follows the same robust pattern.Like
RunScriptValidate, this function recomputes the script hash from the unmarshaled script rather than trusting the input file's hash field.docs/swagger.json (2)
22-150: Script endpoint path specs look consistent with handlers.The new
/api/script/address,/api/script/create, and/api/script/validatepaths correctly reference the correspondingapi.Script*Request/Responsemodels and use POST with the same summaries/descriptions as ininternal/api/api.go. No issues from a contract/shape perspective.
362-507: Script-related definitions andbursa.KeyFile/bursa.Walletupdates are coherent.
api.Script*definitions (request/response) match the JSON tags and types used in the Go models and handlers, includingscriptHashnaming andtimelock_*/slotasintegerwithformat: int64.api.ScriptValidateRequestonly requiresscript, mirroring the now-optionalNetworkfield in the handler.- Updated
bursa.KeyFile/bursa.Walletschemas (examples removed, types kept) remain compatible with existing handlers.Looks good as the canonical Swagger source for the new script functionality.
Also applies to: 603-678
openapi/model_api_script_create_request.go (1)
20-292: ApiScriptCreateRequest model and JSON validation look correct.The generated model matches the Swagger schema:
key_hashes,network, andtypeare enforced as required inUnmarshalJSON, whilerequiredand timelock fields stay optional with pointer types.DisallowUnknownFieldsis a nice touch to keep payloads strict.openapi/model_api_script_validate_response.go (1)
18-237: ApiScriptValidateResponse matches the documented response shape.Pointer fields with
omitemptyandToMap()-based marshalling align with the Swagger definition (scriptHash,signatures,slot,valid), and will happily consume the server’s always-present values as non-nil pointers. Looks fine.openapi/model_api_script_response.go (1)
18-235: ApiScriptResponse struct aligns withapi.ScriptResponseschema.Address/script/scriptHash/type fields and JSON tags match the Swagger definition. Nullable wrappers and
ToMap()follow the usual openapi-generator pattern; nothing problematic here.openapi/model_api_script_address_request.go (1)
20-195: ApiScriptAddressRequest JSON handling is strict and consistent.Required-property checks for
networkandscriptplusDisallowUnknownFieldsgive you a tight client-side model that matches the Swagger definition. Server-side network validation via enums/validators can stay as-is.docs/docs.go (1)
26-154: Swag template correctly mirrors new script API surface.The
/api/script/address|create|validatepaths and theapi.Script*definitions indocTemplatematch the JSON Swagger spec and the handler structs ininternal/api/api.go(network enums, timelock formats,scriptHashnaming, etc.).bursa.KeyFile/bursa.Walletare also aligned. Regeneration pipeline looks coherent.Also applies to: 366-511, 607-682
internal/api/api.go (2)
535-540: WalletRestore error response now safely JSON-encodeserr.Error().Wrapping the error string in a
map[string]stringand marshaling withjson.Marshalavoids JSON injection/malformed responses while still surfacing a detailed client error, with a reasonable fallback message if marshaling fails. This is a solid hardening over direct string interpolation.
1171-1267: handleScriptAddress correctly validates input and returns a typed response.
- Validates
script+network(mainnet|testnet) before processing.- Reuses
bursa.UnmarshalScriptthenbursa.GetScriptAddressandbursa.GetScriptHash, returningScriptAddressResponsewithaddress,network, and hex-encodedscriptHash, in line with the Swagger and OpenAPI models.- Error responses for invalid JSON, validation failures, bad script format, address-generation errors, and hash errors are consistently JSON-encoded with safe marshaling.
No functional issues spotted here.
openapi/model_api_script_validate_request.go (2)
20-30: LGTM! Slot field correctly uses int64.The Slot field now uses
*int64, which correctly addresses the previous comment about Cardano slot numbers potentially overflowing int32 (max 2,147,483,647). This aligns with the OpenAPI spec'sformat: int64specification.
234-272: LGTM! Proper required field and unknown field validation.The UnmarshalJSON implementation correctly validates that required fields are present and rejects unknown fields, following OpenAPI best practices for strict schema enforcement.
openapi/model_bursa_wallet.go (1)
23-41: LGTM! Consistent implementation of new wallet key fields.The nine new committee and drep key fields are implemented consistently with existing Payment/Stake field patterns. All accessor methods (Get/GetOk/Has/Set) are complete, and ToMap correctly handles optional field serialization.
Also applies to: 60-346, 646-672
openapi/api/openapi.yaml (1)
16-108: LGTM! Script endpoints follow consistent API patterns.The three new script endpoints (/address, /create, /validate) follow the established pattern for the API with consistent request/response structures and appropriate HTTP status codes.
openapi/docs/BursaWallet.md (1)
7-269: LGTM! Documentation accurately reflects expanded BursaWallet model.The documentation correctly describes the nine new committee and drep key fields along with their accessor methods, maintaining consistency with the existing field documentation patterns.
| // NewScriptBefore creates a "before" script valid before the given slot | ||
| func NewScriptBefore(slot uint64) *NativeScript { | ||
| concrete := &NativeScriptInvalidBefore{ | ||
| Type: 4, | ||
| Slot: slot, | ||
| } | ||
| cborData, err := cbor.Encode(concrete) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| var script NativeScript | ||
| if _, err := cbor.Decode(cborData, &script); err != nil { | ||
| return nil | ||
| } | ||
| return &script | ||
| } | ||
|
|
||
| // NewScriptAfter creates an "after" script valid after the given slot | ||
| func NewScriptAfter(slot uint64) *NativeScript { | ||
| concrete := &NativeScriptInvalidHereafter{ | ||
| Type: 5, | ||
| Slot: slot, | ||
| } | ||
| cborData, err := cbor.Encode(concrete) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| var script NativeScript | ||
| if _, err := cbor.Decode(cborData, &script); err != nil { | ||
| return nil | ||
| } | ||
| return &script | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Cardano CIP NativeScriptInvalidBefore InvalidHereafter type 4 type 5 semantics
💡 Result:
Summary — native-script constructor tags and semantics
-
Constructor tags (CBOR/Phase‑1 native scripts): 0 = script_pubkey (sig), 1 = script_all (all), 2 = script_any (any), 3 = script_n_of_k (n-of-k), 4 = invalid_before, 5 = invalid_hereafter. [1][2]
-
Semantics of types 4 and 5:
- invalid_before (tag 4): specifies the left (included) endpoint a of the validity interval — the transaction slot must be >= a.
- invalid_hereafter (tag 5): specifies the right (excluded) endpoint b of the validity interval — the transaction slot must be < b.
- Together they define a half‑open interval [a, b) for when the script can succeed. Values are expressed as Cardano slot numbers. [2][3]
Sources
- CIP‑0029 (Phase‑1 native scripts specification / CBOR/CDDL). [cited for overall constructor tags]. [1]
- Cardano StackExchange explanation quoting the CDDL showing script_pubkey..invalid_before..invalid_hereafter and the half‑open interval comment. [2]
- Native‑script docs / libraries (pycardano / cardano‑c) showing InvalidBefore / InvalidHereAfter constructors and slot semantics. [3]
Timelock function implementations are semantically inverted from their names.
According to Cardano CIP-0029, the native script types have these meanings:
NativeScriptInvalidBefore(type 4): Script succeeds when slot >= specified slot (valid at or after)NativeScriptInvalidHereafter(type 5): Script succeeds when slot < specified slot (valid before)
The current implementation swaps these:
NewScriptBeforeusesNativeScriptInvalidBefore(type 4) → creates a script valid after the slotNewScriptAfterusesNativeScriptInvalidHereafter(type 5) → creates a script valid before the slot
This contradicts the documented function behavior. Either swap the type assignments to match the function names, or rename the functions to NewScriptValidAfter/NewScriptValidBefore to match their actual semantics.
🤖 Prompt for AI Agents
In bursa.go around lines 953 to 985, the timelock creators are semantically
inverted: NewScriptBefore currently constructs a NativeScriptInvalidBefore (type
4) which per CIP-0029 is valid at/after the slot, and NewScriptAfter constructs
NativeScriptInvalidHereafter (type 5) which is valid before the slot; fix by
swapping the types used (or swap the concrete struct types used in each
function) so NewScriptBefore produces a script that is valid before the given
slot (use NativeScriptInvalidHereafter/type 5) and NewScriptAfter produces a
script that is valid after the given slot (use NativeScriptInvalidBefore/type
4), keeping CBOR encode/decode flow and error handling unchanged.
| // validateScriptBefore checks if current slot is before the specified slot | ||
| func validateScriptBefore(script *NativeScriptInvalidBefore, slot uint64) bool { | ||
| return slot < script.Slot | ||
| } | ||
|
|
||
| // validateScriptAfter checks if current slot is after the specified slot | ||
| func validateScriptAfter( | ||
| script *NativeScriptInvalidHereafter, | ||
| slot uint64, | ||
| ) bool { | ||
| return slot >= script.Slot | ||
| } |
There was a problem hiding this comment.
Validator logic matches inverted naming convention.
These validators are consistent with the current naming but will need updates if the timelock function naming is corrected per the previous comment.
Note: validateScriptBefore checking slot < script.Slot with NativeScriptInvalidBefore is semantically checking "is current slot before the 'valid after' threshold" which returns true when the script is not yet valid. This appears inverted from expected behavior.
🤖 Prompt for AI Agents
bursa.go lines 1248-1259: The validator logic is inverted for the timelock
checks — NativeScriptInvalidBefore should be considered valid when current slot
is at or after the script.Slot, and NativeScriptInvalidHereafter should be valid
when current slot is before the script.Slot; change validateScriptBefore to
return slot >= script.Slot and change validateScriptAfter to return slot <
script.Slot (and adjust any related tests or callers accordingly).
| // handleScriptCreate godoc | ||
| // | ||
| // @Summary Create a multi-signature script | ||
| // @Description Create a new multi-signature script with the specified parameters | ||
| // @Accept json | ||
| // @Produce json | ||
| // @Param request body ScriptCreateRequest true "Script Create Request" | ||
| // @Success 200 {object} ScriptResponse "Script successfully created" | ||
| // @Failure 400 {string} string "Invalid request" | ||
| // @Failure 500 {string} string "Internal server error" | ||
| // @Router /api/script/create [post] | ||
| func handleScriptCreate(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodPost { | ||
| http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) | ||
| return | ||
| } | ||
|
|
||
| var req ScriptCreateRequest | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| _, _ = w.Write([]byte(`{"error":"Invalid JSON request"}`)) | ||
| return | ||
| } | ||
|
|
||
| // Validate the request | ||
| if err := validate.Struct(req); err != nil { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| var validationErrors validator.ValidationErrors | ||
| if errors.As(err, &validationErrors) { | ||
| _, _ = fmt.Fprintf( | ||
| w, | ||
| `{"error":"Validation failed","fields":%s}`, | ||
| formatValidationErrors(validationErrors), | ||
| ) | ||
| } else { | ||
| _, _ = w.Write([]byte(`{"error":"Invalid request format"}`)) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| logger := logging.GetLogger() | ||
|
|
||
| // Parse key hashes | ||
| hashes := make([][]byte, len(req.KeyHashes)) | ||
| for i, hashStr := range req.KeyHashes { | ||
| hash, err := hex.DecodeString(hashStr) | ||
| if err != nil { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| errorResp := map[string]string{ | ||
| "error": "Invalid key hash format: " + hashStr, | ||
| } | ||
| if jsonBytes, jsonErr := json.Marshal(errorResp); jsonErr == nil { | ||
| _, _ = w.Write(jsonBytes) | ||
| } else { | ||
| _, _ = w.Write([]byte(`{"error":"Invalid key hash format"}`)) | ||
| } | ||
| return | ||
| } | ||
| hashes[i] = hash | ||
| } | ||
|
|
||
| // Create the script | ||
| var script bursa.Script | ||
| var err error | ||
| switch req.Type { | ||
| case "nOf": | ||
| if req.Required <= 0 || req.Required > len(hashes) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| _, _ = w.Write( | ||
| []byte(`{"error":"Invalid required signatures count"}`), | ||
| ) | ||
| return | ||
| } | ||
| script, err = bursa.NewMultiSigScript(req.Required, hashes...) | ||
| case "all": | ||
| script, err = bursa.NewAllMultiSigScript(hashes...) | ||
| case "any": | ||
| script, err = bursa.NewAnyMultiSigScript(hashes...) | ||
| default: | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| errorResp := map[string]string{ | ||
| "error": "Unsupported script type: " + req.Type, | ||
| } | ||
| if jsonBytes, jsonErr := json.Marshal(errorResp); jsonErr == nil { | ||
| _, _ = w.Write(jsonBytes) | ||
| } else { | ||
| _, _ = w.Write([]byte(`{"error":"Invalid script type"}`)) | ||
| } | ||
| return | ||
| } | ||
| if err != nil { | ||
| logger.Error("failed to create script", "error", err) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| errorResp := map[string]string{"error": err.Error()} | ||
| if jsonBytes, jsonErr := json.Marshal(errorResp); jsonErr == nil { | ||
| _, _ = w.Write(jsonBytes) | ||
| } else { | ||
| _, _ = w.Write([]byte(`{"error":"Failed to create script"}`)) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // Apply timelock if specified | ||
| if req.TimelockBefore > 0 && req.TimelockAfter > 0 { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| errorResp := map[string]string{ | ||
| "error": "cannot specify both timelock_before and timelock_after", | ||
| } | ||
| if jsonBytes, jsonErr := json.Marshal(errorResp); jsonErr == nil { | ||
| _, _ = w.Write(jsonBytes) | ||
| } else { | ||
| _, _ = w.Write([]byte(`{"error":"Invalid timelock parameters"}`)) | ||
| } | ||
| return | ||
| } | ||
| if req.TimelockBefore > 0 { | ||
| script = bursa.NewTimelockedScript(req.TimelockBefore, true, script) | ||
| } else if req.TimelockAfter > 0 { | ||
| script = bursa.NewTimelockedScript(req.TimelockAfter, false, script) | ||
| } | ||
|
|
||
| // Marshal script data | ||
| scriptData, err := bursa.MarshalScript(script, req.Network) | ||
| if err != nil { | ||
| logger.Error("failed to marshal script", "error", err) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| _, _ = w.Write([]byte(`{"error":"Internal server error"}`)) | ||
| return | ||
| } | ||
|
|
||
| // Return ScriptResponse with consistent field naming | ||
| response := ScriptResponse{ | ||
| Type: scriptData.Type, | ||
| Script: scriptData.Script, | ||
| Address: scriptData.Address, | ||
| ScriptHash: scriptData.Hash, | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| resp, err := json.Marshal(response) | ||
| if err != nil { | ||
| logger.Error("failed to serialize script response", "error", err) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| _, _ = w.Write([]byte(`{"error":"Internal server error"}`)) | ||
| return | ||
| } | ||
| _, _ = w.Write(resp) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ScriptCreateRequest validation usage
rg -n "ScriptCreateRequest" internal/api/api.go
rg -n "validate.Struct\(req\)" internal/api/api.go -C3Repository: blinklabs-io/bursa
Length of output: 2029
🏁 Script executed:
#!/bin/bash
# Check ScriptCreateRequest struct definition and validation tags
sed -n '133,200p' internal/api/api.goRepository: blinklabs-io/bursa
Length of output: 3211
🏁 Script executed:
#!/bin/bash
# Check validator library import and version
rg -n "import" internal/api/api.go | head -20
# Check for validator library
rg -n "validator" internal/api/api.go | head -10
# Check formatValidationErrors function
rg -n "formatValidationErrors" internal/api/api.go -B2 -A5Repository: blinklabs-io/bursa
Length of output: 2719
🏁 Script executed:
#!/bin/bash
# Check if bursa module functions exist
rg -n "func.*NewMultiSigScript|func.*NewAllMultiSigScript|func.*NewAnyMultiSigScript|func.*NewTimelockedScript|func.*MarshalScript"
# Also check import of bursa module
rg -n '".*bursa' internal/api/api.goRepository: blinklabs-io/bursa
Length of output: 961
🏁 Script executed:
#!/bin/bash
# Check exact line numbers of handleScriptCreate function
rg -n "^func handleScriptCreate" internal/api/api.go
# Get the function boundaries and verify timelock logic
sed -n '898,1054p' internal/api/api.go | grep -n "timelock\|func" | head -20Repository: blinklabs-io/bursa
Length of output: 383
🏁 Script executed:
#!/bin/bash
# Get full handleScriptCreate function (from line 909 to next function or end)
sed -n '909,1060p' internal/api/api.go | tail -100
# Check what comes after line 1054 to see if function ends there
sed -n '1050,1065p' internal/api/api.goRepository: blinklabs-io/bursa
Length of output: 3689
🏁 Script executed:
#!/bin/bash
# Check the exact Required validation tag syntax
sed -n '134,142p' internal/api/api.go
# Also check if there are any tests or examples of how required_if is used
rg -n "required_if" internal/api/api.goRepository: blinklabs-io/bursa
Length of output: 922
🏁 Script executed:
#!/bin/bash
# Check go-playground/validator v10 documentation or check if there are any go.mod references
cat go.mod | grep validator
# Also check if there are tests that validate the Required field
rg -n "Required.*nOf\|required.*nOf" --type goRepository: blinklabs-io/bursa
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Check for test files related to handleScriptCreate
find . -name "*test*.go" -type f | xargs grep -l "handleScriptCreate\|ScriptCreateRequest" 2>/dev/null
# Also check for any comments or issues related to the validation
rg -n "required_if.*Type\|TODO.*required\|FIXME.*required" --type goRepository: blinklabs-io/bursa
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check api_test.go for ScriptCreateRequest tests
rg -n "ScriptCreateRequest\|handleScriptCreate\|Test.*Script.*Create" internal/api/api_test.go -A5 | head -50Repository: blinklabs-io/bursa
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Let's verify the key code sections match the review claims
# Check the Required validation in context of the nOf case
sed -n '940,960p' internal/api/api.go
# Check the timelock validation and application
sed -n '1000,1030p' internal/api/api.goRepository: blinklabs-io/bursa
Length of output: 1660
🏁 Script executed:
#!/bin/bash
# Check the actual Required field validation in the nOf case
# The handler validates Required at runtime with: if req.Required <= 0 || req.Required > len(hashes)
sed -n '955,970p' internal/api/api.go
# Also check if there's any documentation or reference to the required_if issue
rg -n "required_if\|Required.*nOf\|nOf.*Required" . --type go | grep -v node_modulesRepository: blinklabs-io/bursa
Length of output: 431
🏁 Script executed:
#!/bin/bash
# Let's verify the complete validation flow one more time
# Check if there are any related issues or TODOs
rg -n "TODO\|FIXME\|XXX\|HACK" internal/api/api.go | grep -i "required\|valid\|tag"
# Check if the validator is actually called with the struct
sed -n '920,940p' internal/api/api.goRepository: blinklabs-io/bursa
Length of output: 581
handleScriptCreate implementation is solid, but the Required field validation tag needs correction.
- Request validation mirrors the Swagger/OpenAPI contract and reuses the shared
formatValidationErrorshelper. - Key hashes are decoded via
hex.DecodeStringwith per-hash 400s and JSON-encoded error messages, avoiding interpolation issues. - Script construction correctly branches on
Type(nOf/all/any) and validatesRequiredvs number of hashes in thenOfcase via runtime check. - The timelock logic correctly rejects requests that specify both
timelock_beforeandtimelock_after, then wraps the script with a single before/after timelock as expected. - Response mapping via
bursa.MarshalScriptintoScriptResponsekeeps JSON field names (scriptHashvshash) consistent with the documented API.
However, the Required field validation tag validate:"required_if=Type nOf,omitempty,min=1" contains omitempty, which contradicts the required_if condition—the omitempty tag allows zero values even when Type is nOf. Remove omitempty and verify the required_if syntax is correct for go-playground/validator v10 (likely required_if=Type=nOf).
🤖 Prompt for AI Agents
internal/api/api.go lines 898-1054: the struct tag for Required mistakenly
includes `omitempty` which defeats the `required_if` rule and the `required_if`
syntax may be wrong; remove `omitempty` and update the tag to a valid
go-playground/validator v10 form (e.g. `validate:"required_if=Type nOf,min=1"`
or `validate:"required_if=Type=nOf,min=1"` depending on your validator version),
then run tests or a quick validation check to ensure the rule enforces presence
when Type is "nOf".
Summary by cubic
Adds CIP-1854 multi-signature support with native scripts, key derivation, hashing, and address generation. Adds CLI and API endpoints to create, validate, and derive addresses for scripts.
Written for commit 37830d9. Summary will update automatically on new commits.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.