Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds proof-based intent lookup and multi-intent responses: new GetIntent message type and parsing, repository methods and DB indexes for querying intents by proof, service-level proof verification/matching, updated gRPC/OpenAPI/proto contracts, and tests for parsing and repo behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as gRPC Handler
participant Service as Service Layer
participant Verifier as Proof Verifier
participant Repo as Repository
participant DB as Database
Client->>Handler: GetIntent(txid) or GetIntent(proof+message)
Handler->>Handler: parse txid or parseGetIntent(proof,message)
alt txid path
Handler->>Service: GetIntentByTxid(txid)
Service->>Repo: GetIntentByTxid(txid)
Repo->>DB: SELECT ... WHERE txid=?
DB-->>Repo: intent row
Repo-->>Service: Intent
else proof+message path
Handler->>Service: GetIntentByProofs(proof, message)
Service->>Verifier: verifyIntentProofAndFindMatches(proof, message)
Verifier->>Repo: GetIntentsByProof(proof)
Repo->>DB: SELECT ... WHERE proof=?
DB-->>Repo: intent rows
Repo-->>Verifier: []*Intent
Verifier->>Verifier: validate proof/message and filter matches
Verifier-->>Service: []Intent matches
end
Service-->>Handler: GetIntentResponse(intent/intents)
Handler-->>Client: return response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
isn't duplicated of #890 ? |
![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 (3)
api-spec/openapi/swagger/ark/v1/service.openapi.json (2)
936-949:⚠️ Potential issue | 🟡 Minor
GetIntentResponsehas bothintentandintentswith no documentation distinguishing their semantics.Clients cannot tell from the spec alone which field is populated and under what conditions (txid path vs. proof path). At minimum, add
descriptionfields to both properties.📝 Suggested descriptions
"GetIntentResponse": { "title": "GetIntentResponse", "type": "object", "properties": { "intent": { - "$ref": "#/components/schemas/Intent" + "$ref": "#/components/schemas/Intent", + "description": "Populated when querying by txid. Contains the single matching intent." }, "intents": { "type": "array", + "description": "Populated when querying by proof. Contains all intents whose inputs are owned by the proof signer.", "items": { "$ref": "#/components/schemas/Intent" } } } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-spec/openapi/swagger/ark/v1/service.openapi.json` around lines 936 - 949, The GetIntentResponse schema currently exposes both properties intent and intents without descriptions; update the OpenAPI component "GetIntentResponse" to add explicit description fields for "intent" and "intents" clarifying their semantics (e.g., "intent" — single Intent returned for txid path lookups; "intents" — array of Intent results returned for proof/path or batch queries) so clients know which is populated under which conditions; edit the properties for GetIntentResponse in the schema to include these description strings for intent and intents.
413-418:⚠️ Potential issue | 🟡 MinorAdd an
operationIddescription for the GetIntent endpoint.The
/v1/intentGET operation is the only endpoint in this spec lacking adescription. With this PR it now serves two distinct filter paths (txid-based and proof-based), making inline documentation especially important for SDK generators and API consumers.📝 Suggested addition
"operationId": "ArkService_GetIntent", + "description": "GetIntent retrieves a registered intent by either a txid (exact match) or an Intent containing a BIP-322 proof (ownership-based lookup). Exactly one of txid or intent must be provided.", "parameters": [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-spec/openapi/swagger/ark/v1/service.openapi.json` around lines 413 - 418, Add a descriptive "description" field to the GET /v1/intent operation (operationId "ArkService_GetIntent") that explains the two supported filter modes: txid-based lookup (provide "txid" to retrieve intent by transaction id) and proof-based lookup (provide proof parameters to validate and fetch intent), and clarify which query parameters are required for each path and expected behavior/response when no match is found; place this description alongside the existing "operationId" to aid SDK generators and API consumers.internal/core/application/service.go (1)
2134-2234:⚠️ Potential issue | 🔴 CriticalNil dereference on
psbtInput.WitnessUtxo— potential server panic / DoS
psbt.PInput.WitnessUtxois*wire.TxOutand is optional per the PSBT spec. A proof PSBT submitted withoutWitnessUtxoset on any input will panic at:
Line Access 2173 psbtInput.WitnessUtxo.PkScript(boarding path)2181 psbtInput.WitnessUtxo.Value(boarding path)2194 psbtInput.WitnessUtxo.Value(vtxo path)2227 psbtInput.WitnessUtxo.PkScript(vtxo path)The established guard in
RegisterIntent(line 1521) prevents exactly this:if psbtInput.WitnessUtxo == nil { return "", errors.INVALID_PSBT_INPUT.New(...) }The same guard is absent here, and this code is now reachable from the new public
GetIntentByProofsRPC endpoint, making it exploitable for a remote panic/DoS.Add the nil check immediately after the taproot-leaf check on line 2137:
🛡️ Proposed fix
if len(psbtInput.TaprootLeafScript) == 0 { return nil, errors.INVALID_PSBT_INPUT.New("missing taproot leaf script on input %d", i+1). WithMetadata(errors.InputMetadata{Txid: proofTxid, InputIndex: i + 1}) } + + if psbtInput.WitnessUtxo == nil { + return nil, errors.INVALID_PSBT_INPUT.New("missing witness utxo on input %d", i+1). + WithMetadata(errors.InputMetadata{Txid: proofTxid, InputIndex: i + 1}) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/service.go` around lines 2134 - 2234, The code dereferences psbtInput.WitnessUtxo (a *wire.TxOut) without a nil check, which can panic when called from GetIntentByProofs; add a nil guard immediately after the existing taproot leaf script check (the block handling psbtInput.TaprootLeafScript) that returns errors.INVALID_PSBT_INPUT.New(...) with appropriate InputMetadata if psbtInput.WitnessUtxo == nil, mirroring the guard used in RegisterIntent, so subsequent uses of psbtInput.WitnessUtxo.PkScript and .Value are safe.
🧹 Nitpick comments (9)
api-spec/protobuf/ark/v1/service.proto (1)
333-336:GetIntentResponseuses two separate fields for the two lookup paths.
intent(field 1) is populated for txid-based lookups, whileintents(field 2) is populated for proof-based lookups. Callers must inspect both and correlate back to the original request variant. Prefer a singlerepeated Intent intents = 2for both paths (returning at most one element for the txid case) and mark field 1 deprecated, eliminating the ambiguity.♻️ Proposed change
message GetIntentResponse { - Intent intent = 1; + Intent intent = 1 [deprecated = true]; // kept for wire-compat; prefer intents repeated Intent intents = 2; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-spec/protobuf/ark/v1/service.proto` around lines 333 - 336, GetIntentResponse currently uses two separate fields (intent field 1 for txid lookups and repeated Intent intents field 2 for proof lookups) causing caller ambiguity; change the proto so callers always use repeated Intent intents for both paths (for txid lookups return at most one element), mark the single Intent field (field name intent, number 1) deprecated with option deprecated = true, remove server/client logic that sets/reads the deprecated intent field and instead populate and read the repeated intents field, and regenerate client/server stubs and update any handlers that previously relied on GetIntentResponse.intent to use GetIntentResponse.intents[0] when expecting a single result.internal/interface/grpc/handlers/parser.go (1)
87-103:parseGetIntentis the fourth copy of an identical three-step pattern.
parseRegisterIntent,parseEstimateFeeIntent,parseDeleteIntent,parseGetPendingTxIntent, and nowparseGetIntentall share the same structure. A generic helper would reduce the pattern to a single implementation:♻️ Proposed refactor (Go 1.18+)
+type intentMessageDecoder[T any] interface { + Decode(string) error + *T +} + +func parseTypedIntent[T any, P intentMessageDecoder[T]]( + intentProof *arkv1.Intent, + errMsg string, +) (*intent.Proof, *T, error) { + proof, err := parseIntentProofTx(intentProof) + if err != nil { + return nil, nil, err + } + if len(intentProof.GetMessage()) <= 0 { + return nil, nil, fmt.Errorf("missing message") + } + var msg T + if err := P(&msg).Decode(intentProof.GetMessage()); err != nil { + return nil, nil, fmt.Errorf("%s", errMsg) + } + return proof, &msg, nil +} + -func parseGetIntent( - intentProof *arkv1.Intent, -) (*intent.Proof, *intent.GetIntentMessage, error) { - proof, err := parseIntentProofTx(intentProof) - if err != nil { - return nil, nil, err - } - if len(intentProof.GetMessage()) <= 0 { - return nil, nil, fmt.Errorf("missing message") - } - var message intent.GetIntentMessage - if err := message.Decode(intentProof.GetMessage()); err != nil { - return nil, nil, fmt.Errorf("invalid get-intent message") - } - return proof, &message, nil -} +func parseGetIntent(i *arkv1.Intent) (*intent.Proof, *intent.GetIntentMessage, error) { + return parseTypedIntent[intent.GetIntentMessage, *intent.GetIntentMessage](i, "invalid get-intent message") +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interface/grpc/handlers/parser.go` around lines 87 - 103, Multiple intent parsing functions (parseGetIntent, parseRegisterIntent, parseEstimateFeeIntent, parseDeleteIntent, parseGetPendingTxIntent) duplicate the same three-step pattern; factor them into a single generic helper (e.g., parseIntentWithMessage) that calls parseIntentProofTx, validates GetMessage() length and then decodes into a provided message via a decode function or interface; update each specific function (parseGetIntent, parseRegisterIntent, parseEstimateFeeIntent, parseDeleteIntent, parseGetPendingTxIntent) to call this helper, passing the appropriate message type/decoder and returning the parsed proof and typed message or error (keep references to parseIntentProofTx and the specific message types to locate code).internal/infrastructure/db/postgres/sqlc/query.sql (1)
428-430: No index onproofcolumn — full table scan on every call.The
prooffield is a base64-encoded PSBT (potentially hundreds of bytes). Without a B-tree or hash index onintent.proof, everySelectIntentsByProofcall scans the entireintenttable. Consider adding an index in the corresponding schema migration, or storing a compact identifier (e.g., SHA-256 of the proof bytes) to index instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/postgres/sqlc/query.sql` around lines 428 - 430, SelectIntentsByProof issues full-table scans because intent.proof is large and unindexed; add an index on intent.proof (or better, add a compact hash column like proof_hash and index that) in the DB schema migration so the SELECT id, txid, proof, message FROM intent WHERE proof = `@proof` query can use an index; update any inserts/updates to populate the new proof_hash (e.g., SHA-256 of proof) and change SelectIntentsByProof to query by proof_hash (or keep the original and rely on the new index) so lookups are efficient.pkg/ark-lib/intent/message.go (1)
141-169:GetIntentMessageis structurally identical toDeleteMessageandGetPendingTxMessage.All three types share the exact same fields (
BaseMessage+ExpireAt int64) and the same four methods (Encode,Decode,GetExpireAt,GetBaseMessage), with only the checkedIntentMessageTypeconstant differing. If a fourth message type with the same shape is added later, the duplication grows further.Consider a shared parameterised base:
♻️ Proposed refactor
+// simpleMessage is the shared base for message types that contain only BaseMessage + ExpireAt. +type simpleMessage struct { + BaseMessage + ExpireAt int64 `json:"expire_at"` +} + +func (m simpleMessage) GetExpireAt() int64 { return m.ExpireAt } +func (m simpleMessage) GetBaseMessage() BaseMessage { return m.BaseMessage } + +func (m simpleMessage) encode() (string, error) { + b, err := json.Marshal(m) + if err != nil { + return "", err + } + return string(b), nil +} + type DeleteMessage struct { - BaseMessage - ExpireAt int64 `json:"expire_at"` + simpleMessage } -func (m DeleteMessage) GetExpireAt() int64 { return m.ExpireAt } -func (m DeleteMessage) GetBaseMessage() BaseMessage { return m.BaseMessage } -func (m DeleteMessage) Encode() (string, error) { ... } +func (m DeleteMessage) Encode() (string, error) { return m.encode() } func (m *DeleteMessage) Decode(data string) error { - if err := json.Unmarshal([]byte(data), m); err != nil { return err } - if m.Type != IntentMessageTypeDelete { ... } - return nil + if err := json.Unmarshal([]byte(data), &m.simpleMessage); err != nil { return err } + if m.Type != IntentMessageTypeDelete { return fmt.Errorf("invalid intent message type: %s", m.Type) } + return nil } // Similarly for GetPendingTxMessage and GetIntentMessage.internal/interface/grpc/handlers/arkservice.go (1)
689-692: Useerrinstead ofsvcErrfor idiomatic Go.
intentsis a new variable on the left side so:=can reuseerr.♻️ Proposed fix
- intents, svcErr := h.svc.GetIntentByProofs(ctx, *proof, *message) - if svcErr != nil { - return nil, svcErr + intents, err := h.svc.GetIntentByProofs(ctx, *proof, *message) + if err != nil { + return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interface/grpc/handlers/arkservice.go` around lines 689 - 692, Replace the non-idiomatic variable name svcErr with err by reusing := when calling h.svc.GetIntentByProofs so the call becomes intents, err = ... (or intents, err := ... if appropriate in that scope); update all subsequent references and returns to use err instead of svcErr, referencing the GetIntentByProofs call and the intents variable in arkservice.go/handler to keep naming idiomatic.internal/interface/grpc/handlers/parser_test.go (1)
98-129:TestParseDeleteIntentcovers only one invalid case.
TestParseGetIntentexhaustively testsnil_intent,missing_proof,invalid_proof,missing_message,wrong_message_type, andinvalid_message_json.TestParseDeleteIntentonly testswrong_message_type. IfparseDeleteIntentandparseGetIntentshare the same proof-parsing code path, the other error cases for delete are implicitly covered, but it is not made explicit. Consider mirroring the invalid case set (or at minimum adding a comment explaining the shared code path assumption).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interface/grpc/handlers/parser_test.go` around lines 98 - 129, TestParseDeleteIntent only asserts the "wrong_message_type" invalid case; add the same invalid-case coverage used by TestParseGetIntent (nil_intent, missing_proof, invalid_proof, missing_message, wrong_message_type, invalid_message_json) or document why parseDeleteIntent shares proof-parsing with parseGetIntent; specifically update TestParseDeleteIntent to iterate the full fixtures.ParseDeleteIntent.Invalid set (or reuse fixtures.ParseGetIntent.Invalid) and assert error messages via tc.ExpectedError, or add a concise comment above the "invalid" subtest referencing the shared proof-parsing code path (parseDeleteIntent) to make the assumption explicit.internal/infrastructure/db/sqlite/sqlc/query.sql (1)
431-433: Add an index onintent.proofto avoid full-table scans inSelectIntentsByProof.The query performs a sequential scan on the
intenttable without an index on theproofcolumn. For deployments with many historical rounds, this degrades performance over time. Note thatSelectIntentByTxid(which queries ontxid) is similarly unindexed and could benefit from the same treatment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/sqlite/sqlc/query.sql` around lines 431 - 433, The query SelectIntentsByProof causes full-table scans because there is no index on intent.proof; add a migration that creates an index on the intent.proof column (e.g., CREATE INDEX CONCURRENTLY or CREATE INDEX IF NOT EXISTS on intent(proof)) to speed SelectIntentsByProof, and while here also add an index on intent.txid to optimize SelectIntentByTxid; update schema/migration files and, if you have DB setup/test fixtures, run them to ensure the new indexes are created and queries use them.api-spec/openapi/swagger/ark/v1/service.openapi.json (1)
924-934:GetIntentRequestshould expressoneOfexclusivity betweentxidandintent.The proto definition uses
oneof filter { string txid = 1; Intent intent = 2; }, meaning only one field should be sent. The OpenAPI schema allows both to be present simultaneously without any validation constraint, leaving server-side conflict resolution undefined for clients.♻️ Proposed OpenAPI `oneOf` modelling
"GetIntentRequest": { "title": "GetIntentRequest", "type": "object", - "properties": { - "intent": { - "$ref": "#/components/schemas/Intent" - }, - "txid": { - "type": "string" - } - } + "oneOf": [ + { + "properties": { + "txid": { "type": "string" } + }, + "required": ["txid"] + }, + { + "properties": { + "intent": { "$ref": "#/components/schemas/Intent" } + }, + "required": ["intent"] + } + ] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-spec/openapi/swagger/ark/v1/service.openapi.json` around lines 924 - 934, The GetIntentRequest schema currently allows both intent and txid simultaneously; modify the OpenAPI definition for GetIntentRequest to enforce mutual exclusivity by replacing the current properties-only schema with a oneOf construct that defines two object variants: one object with required txid (type: string) and no intent, and another object with required intent (ref: `#/components/schemas/Intent`) and no txid; ensure titles/descriptions remain and update any examples/validators to use the new oneOf-based schema so clients cannot send both fields together.internal/core/application/service.go (1)
4164-4181: Naming:GetIntentByProofsimplies multiple proofs as input;GetIntentsByProofis more idiomaticThe function takes a single
intent.Proofand returns multiple[]*domain.Intent. The plural belongs on the output noun, consistent with the existing siblingDeleteIntentsByProof. The current name suggests multiple proofs are supplied (likeGetIntentByTxids), which is misleading.♻️ Proposed rename
-func (s *service) GetIntentByProofs( +func (s *service) GetIntentsByProof( ctx context.Context, proof intent.Proof, message intent.GetIntentMessage, ) ([]*domain.Intent, errors.Error) {The corresponding declaration in
internal/core/application/types.goand any RPC handler that calls this method would also need updating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/service.go` around lines 4164 - 4181, Rename the method GetIntentByProofs to GetIntentsByProof to correctly reflect that it accepts a single intent.Proof and returns multiple []*domain.Intent; update the function signature in the service implementation (function GetIntentByProofs), its declaration in internal/core/application/types.go, and all call sites including RPC handlers that invoke GetIntentByProofs (they will call verifyIntentProofAndFindMatches unchanged). Ensure names are consistently updated across imports and interfaces to avoid build breaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api-spec/protobuf/ark/v1/service.proto`:
- Around line 326-330: The GetIntentRequest's oneof filter (fields txid and
intent) is being exposed via an HTTP GET binding which forces the intent variant
(Intent field carrying a base64 PSBT proof) into URL query parameters; update
the service.proto HTTP binding for the GetIntent RPC so large proof payloads are
sent in the request body instead of as query params: locate the rpc GetIntent
and its HTTP option that currently maps to get: "/v1/intent" and change it to a
POST-style binding (or add body: "*" / body: "intent") so the Intent oneof
variant (field name intent) is transmitted in the request body while preserving
txid query semantics for small requests. Ensure the change references
GetIntentRequest, the oneof named filter, and the fields txid and intent.
In `@internal/infrastructure/db/badger/ark_repo.go`:
- Around line 572-591: GetIntentsByProof currently calls findRound(ctx, nil)
which deserializes every round; instead add a new secondary index type
IntentProofIndex { Proof string; RoundId string; IntentId string } and maintain
it in addOrUpdateRound alongside the existing IntentIndex (used by
GetIntentByTxid) by upserting one IntentProofIndex entry per intent keyed by
proof; then change GetIntentsByProof to query the IntentProofIndex for the given
proof, retrieve the targeted RoundId/IntentId entries and load only those
specific intents (or their parent rounds) rather than scanning all rounds.
In `@internal/infrastructure/db/postgres/sqlc/queries/query.sql.go`:
- Around line 494-497: The SelectIntentsByProof query (SELECT ... FROM intent
WHERE proof = $1) will cause full table scans; add a new DB migration that
creates an index on the intent.proof column (e.g., CREATE INDEX idx_intent_proof
ON intent(proof)) so SelectIntentsByProof / GetIntentsByProof use the index;
implement the migration file, run it in migration tooling, and ensure the
migration name references the intent_proof index so reviewers can locate it
alongside the query in query.sql.go.
In `@internal/interface/grpc/handlers/arkservice.go`:
- Around line 680-700: The proof-based GetIntentRequest branch (handled after
parseGetIntent and the call to h.svc.GetIntentByProofs) only populates
GetIntentResponse.Intents, causing response.Intent to be nil for single-result
lookups; update the return path so that after building protoIntents you set the
legacy Intent field when len(protoIntents) == 1 (i.e., return
&arkv1.GetIntentResponse{Intent: protoIntents[0], Intents: protoIntents}, nil)
to mirror the txid case and preserve backward compatibility.
---
Outside diff comments:
In `@api-spec/openapi/swagger/ark/v1/service.openapi.json`:
- Around line 936-949: The GetIntentResponse schema currently exposes both
properties intent and intents without descriptions; update the OpenAPI component
"GetIntentResponse" to add explicit description fields for "intent" and
"intents" clarifying their semantics (e.g., "intent" — single Intent returned
for txid path lookups; "intents" — array of Intent results returned for
proof/path or batch queries) so clients know which is populated under which
conditions; edit the properties for GetIntentResponse in the schema to include
these description strings for intent and intents.
- Around line 413-418: Add a descriptive "description" field to the GET
/v1/intent operation (operationId "ArkService_GetIntent") that explains the two
supported filter modes: txid-based lookup (provide "txid" to retrieve intent by
transaction id) and proof-based lookup (provide proof parameters to validate and
fetch intent), and clarify which query parameters are required for each path and
expected behavior/response when no match is found; place this description
alongside the existing "operationId" to aid SDK generators and API consumers.
In `@internal/core/application/service.go`:
- Around line 2134-2234: The code dereferences psbtInput.WitnessUtxo (a
*wire.TxOut) without a nil check, which can panic when called from
GetIntentByProofs; add a nil guard immediately after the existing taproot leaf
script check (the block handling psbtInput.TaprootLeafScript) that returns
errors.INVALID_PSBT_INPUT.New(...) with appropriate InputMetadata if
psbtInput.WitnessUtxo == nil, mirroring the guard used in RegisterIntent, so
subsequent uses of psbtInput.WitnessUtxo.PkScript and .Value are safe.
---
Nitpick comments:
In `@api-spec/openapi/swagger/ark/v1/service.openapi.json`:
- Around line 924-934: The GetIntentRequest schema currently allows both intent
and txid simultaneously; modify the OpenAPI definition for GetIntentRequest to
enforce mutual exclusivity by replacing the current properties-only schema with
a oneOf construct that defines two object variants: one object with required
txid (type: string) and no intent, and another object with required intent (ref:
`#/components/schemas/Intent`) and no txid; ensure titles/descriptions remain and
update any examples/validators to use the new oneOf-based schema so clients
cannot send both fields together.
In `@api-spec/protobuf/ark/v1/service.proto`:
- Around line 333-336: GetIntentResponse currently uses two separate fields
(intent field 1 for txid lookups and repeated Intent intents field 2 for proof
lookups) causing caller ambiguity; change the proto so callers always use
repeated Intent intents for both paths (for txid lookups return at most one
element), mark the single Intent field (field name intent, number 1) deprecated
with option deprecated = true, remove server/client logic that sets/reads the
deprecated intent field and instead populate and read the repeated intents
field, and regenerate client/server stubs and update any handlers that
previously relied on GetIntentResponse.intent to use
GetIntentResponse.intents[0] when expecting a single result.
In `@internal/core/application/service.go`:
- Around line 4164-4181: Rename the method GetIntentByProofs to
GetIntentsByProof to correctly reflect that it accepts a single intent.Proof and
returns multiple []*domain.Intent; update the function signature in the service
implementation (function GetIntentByProofs), its declaration in
internal/core/application/types.go, and all call sites including RPC handlers
that invoke GetIntentByProofs (they will call verifyIntentProofAndFindMatches
unchanged). Ensure names are consistently updated across imports and interfaces
to avoid build breaks.
In `@internal/infrastructure/db/postgres/sqlc/query.sql`:
- Around line 428-430: SelectIntentsByProof issues full-table scans because
intent.proof is large and unindexed; add an index on intent.proof (or better,
add a compact hash column like proof_hash and index that) in the DB schema
migration so the SELECT id, txid, proof, message FROM intent WHERE proof =
`@proof` query can use an index; update any inserts/updates to populate the new
proof_hash (e.g., SHA-256 of proof) and change SelectIntentsByProof to query by
proof_hash (or keep the original and rely on the new index) so lookups are
efficient.
In `@internal/infrastructure/db/sqlite/sqlc/query.sql`:
- Around line 431-433: The query SelectIntentsByProof causes full-table scans
because there is no index on intent.proof; add a migration that creates an index
on the intent.proof column (e.g., CREATE INDEX CONCURRENTLY or CREATE INDEX IF
NOT EXISTS on intent(proof)) to speed SelectIntentsByProof, and while here also
add an index on intent.txid to optimize SelectIntentByTxid; update
schema/migration files and, if you have DB setup/test fixtures, run them to
ensure the new indexes are created and queries use them.
In `@internal/interface/grpc/handlers/arkservice.go`:
- Around line 689-692: Replace the non-idiomatic variable name svcErr with err
by reusing := when calling h.svc.GetIntentByProofs so the call becomes intents,
err = ... (or intents, err := ... if appropriate in that scope); update all
subsequent references and returns to use err instead of svcErr, referencing the
GetIntentByProofs call and the intents variable in arkservice.go/handler to keep
naming idiomatic.
In `@internal/interface/grpc/handlers/parser_test.go`:
- Around line 98-129: TestParseDeleteIntent only asserts the
"wrong_message_type" invalid case; add the same invalid-case coverage used by
TestParseGetIntent (nil_intent, missing_proof, invalid_proof, missing_message,
wrong_message_type, invalid_message_json) or document why parseDeleteIntent
shares proof-parsing with parseGetIntent; specifically update
TestParseDeleteIntent to iterate the full fixtures.ParseDeleteIntent.Invalid set
(or reuse fixtures.ParseGetIntent.Invalid) and assert error messages via
tc.ExpectedError, or add a concise comment above the "invalid" subtest
referencing the shared proof-parsing code path (parseDeleteIntent) to make the
assumption explicit.
In `@internal/interface/grpc/handlers/parser.go`:
- Around line 87-103: Multiple intent parsing functions (parseGetIntent,
parseRegisterIntent, parseEstimateFeeIntent, parseDeleteIntent,
parseGetPendingTxIntent) duplicate the same three-step pattern; factor them into
a single generic helper (e.g., parseIntentWithMessage) that calls
parseIntentProofTx, validates GetMessage() length and then decodes into a
provided message via a decode function or interface; update each specific
function (parseGetIntent, parseRegisterIntent, parseEstimateFeeIntent,
parseDeleteIntent, parseGetPendingTxIntent) to call this helper, passing the
appropriate message type/decoder and returning the parsed proof and typed
message or error (keep references to parseIntentProofTx and the specific message
types to locate code).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/infrastructure/db/sqlite/migration/20260220165109_add_intent_proof_index.up.sql (1)
1-1: Optional: partial index ifproofis nullable.SQLite doesn't support hash indexes (B-tree only), so no change there. But if the
proofcolumn is nullable, addingWHERE proof IS NOT NULLkeeps the index compact — the same rationale as the Postgres migration above.⚡ Proposed optimisation (only if proof is nullable)
-CREATE INDEX IF NOT EXISTS idx_intent_proof ON intent(proof); +CREATE INDEX IF NOT EXISTS idx_intent_proof ON intent(proof) WHERE proof IS NOT NULL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/sqlite/migration/20260220165109_add_intent_proof_index.up.sql` at line 1, The migration creates a full index idx_intent_proof on intent(proof) even though proof may be nullable; if proof is nullable, change the migration to create a partial index by adding a WHERE proof IS NOT NULL predicate to keep the index compact (leave as-is if proof is declared NOT NULL); update the CREATE INDEX statement for idx_intent_proof in the migration file so it uses the WHERE clause when appropriate and run/verify the migration against a test DB.internal/infrastructure/db/postgres/migration/20260220165106_add_intent_proof_index.up.sql (1)
1-1: Remove the WHERE clause from the hash index suggestion since proof is already NOT NULL at the schema level.
GetIntentsByProofperforms only equality comparisons on the proof column. PostgreSQL's hash index provides O(1) lookup vs O(log n) for B-tree and produces a smaller index footprint on TEXT values. Since the proof column is already definedNOT NULL, a partial index is unnecessary.⚡ Proposed optimisation
-CREATE INDEX IF NOT EXISTS idx_intent_proof ON intent(proof); +CREATE INDEX IF NOT EXISTS idx_intent_proof ON intent USING HASH (proof);Note: Hash indexes do not support range or prefix scans. If such queries are needed in the future, revert to B-tree.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/db/postgres/migration/20260220165106_add_intent_proof_index.up.sql` at line 1, Replace the partial/B-tree suggestion with a proper hash index since intent.proof is NOT NULL and GetIntentsByProof uses equality lookups: create a hash index on the proof column (e.g., index name idx_intent_proof_hash) using "USING hash" and remove any WHERE clause; ensure GetIntentsByProof continues to use simple equality comparisons on proof so the hash index can be used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@internal/infrastructure/db/postgres/migration/20260220165106_add_intent_proof_index.up.sql`:
- Line 1: Replace the partial/B-tree suggestion with a proper hash index since
intent.proof is NOT NULL and GetIntentsByProof uses equality lookups: create a
hash index on the proof column (e.g., index name idx_intent_proof_hash) using
"USING hash" and remove any WHERE clause; ensure GetIntentsByProof continues to
use simple equality comparisons on proof so the hash index can be used.
In
`@internal/infrastructure/db/sqlite/migration/20260220165109_add_intent_proof_index.up.sql`:
- Line 1: The migration creates a full index idx_intent_proof on intent(proof)
even though proof may be nullable; if proof is nullable, change the migration to
create a partial index by adding a WHERE proof IS NOT NULL predicate to keep the
index compact (leave as-is if proof is declared NOT NULL); update the CREATE
INDEX statement for idx_intent_proof in the migration file so it uses the WHERE
clause when appropriate and run/verify the migration against a test DB.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/proto.yaml (1)
16-35:⚠️ Potential issue | 🟠 MajorRemoving
verify-clienteliminates the only CI gate that keeps generated code in sync with proto.The deleted job performed
make genrest CI=true(client generation) followed bygit diff --staged --exit-codeto detect uncommitted changes on every proto modification. This caught cases where proto changes were made without regenerating the corresponding client stubs. With it gone, a future proto change that omits regeneration will pass CI silently.The commit message indicates this removal was deliberate ("remove github action for checking client lib stubs"), but provides no rationale. If the job was failing due to stale generated files in this PR, the correct fix is to regenerate the client stubs and restore the job. If there is a deliberate reason to remove it (moving generation elsewhere, replacing with a different enforcement mechanism), that decision should be documented and the replacement gate established before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/proto.yaml around lines 16 - 35, The CI removal removed the verify-client job that ran "make genrest CI=true" and then "git diff --staged --exit-code" to ensure generated client stubs match proto changes; restore that safety or document and replace it. Re-add a verify-client job (or equivalent) that checks out code, runs the genrest generation (make genrest CI=true) and fails if generated files are modified (use git diff --staged --exit-code or equivalent), or if you intentionally moved generation elsewhere, update the workflow to run the new generation/check and add documentation in the PR describing the replacement gate and its location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/proto.yaml:
- Around line 16-35: The CI removal removed the verify-client job that ran "make
genrest CI=true" and then "git diff --staged --exit-code" to ensure generated
client stubs match proto changes; restore that safety or document and replace
it. Re-add a verify-client job (or equivalent) that checks out code, runs the
genrest generation (make genrest CI=true) and fails if generated files are
modified (use git diff --staged --exit-code or equivalent), or if you
intentionally moved generation elsewhere, update the workflow to run the new
generation/check and add documentation in the PR describing the replacement gate
and its location.
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
arkanaai[bot] review — arkd #917
The refactor into verifyIntentProofAndFindMatches is a good separation of concerns — the proof validation code is complex enough that having it shared between DeleteIntentsByProof and the new GetIntent path is the right call.
Protocol correctness ✓
The new GetIntent(intent=<proof>) path uses the same verifyIntentProofAndFindMatches logic as DeleteIntentsByProof. Proof-of-ownership validation (expiry check, taproot leaf script presence, pkscript matching, schnorr sig verification) is fully exercised before any intents are returned. An attacker cannot query someone else's intents without controlling the underlying VTXOs. ✓
verify-client CI job removed
# proto.yaml — this entire job was deleted:
- verify-client:
- name: Check client lib stubs
- ...
- - name: Generate Client
- run: make genrest CI=true
- - name: Check for uncommitted changes
- ...This CI job enforced that the generated gRPC client stubs in pkg/client-lib/ stay in sync with the proto definitions. Removing it means a future proto change (including this one — GetIntentRequest.intent oneof, GetIntentResponse.intents field) could silently diverge from the generated client. This is a regression in CI coverage.
Was this intentional? If the generated client is now out of date, it should be regenerated and the job kept (or replaced with an equivalent check).
Minor — proto change is additive ✓
Adding Intent intent = 2 to the GetIntentRequest oneof and repeated Intent intents = 2 to GetIntentResponse is backwards-compatible. buf breaking will confirm no breakage against the base branch. ✓
Suggestion — the new intents field in GetIntentResponse alongside the existing intent field is slightly awkward. Is intent still populated for the txid-filter path? If so, document which field is populated under which filter case.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api-spec/protobuf/ark/v1/service.proto (1)
165-169: HTTP GET binding with proof payload — previously discussed.The
get: "/v1/intent"binding causes theintentoneof variant (PSBT proof) to be serialized as URL-encoded query parameters. This was flagged in the prior review; adding a POSTadditional_bindingsalongside the existing GET was proposed as a non-breaking opt-in. Marking as previously discussed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-spec/protobuf/ark/v1/service.proto` around lines 165 - 169, The GET binding on rpc GetIntent causes the intent oneof (PSBT proof) to be serialized into query params; add a non-breaking POST alternative by adding an additional_bindings block to the existing option (meshapi.gateway.http) on GetIntent: keep the existing get: "/v1/intent" and add an additional_bindings entry with post: "/v1/intent" and body: "*" (or body: "intent" if you want to limit to that field) so the PSBT/proof can be sent in the request body while preserving the current GET behavior.
🧹 Nitpick comments (2)
api-spec/protobuf/ark/v1/service.proto (2)
327-330: Indentation ofoneof filterbody is inconsistent with every otheroneofblock in this file.All other
oneofblocks use 4-space indentation for their fields; this block uses 6 spaces for fields and 4 spaces for the closing}.♻️ Proposed fix
message GetIntentRequest { oneof filter { - string txid = 1; - Intent intent = 2; - } + string txid = 1; + Intent intent = 2; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-spec/protobuf/ark/v1/service.proto` around lines 327 - 330, The oneof block for `filter` has inconsistent indentation: its fields `txid` and `intent` are indented differently than other oneof blocks; update the `oneof filter` block so that the fields `txid = 1;` and `Intent intent = 2;` use 4-space indentation (matching other oneof blocks) and ensure the closing `}` aligns consistently with those blocks.
333-337: Asymmetric field-population contract makes the response harder to consume correctly.As documented by the comments:
- txid lookup → both
intent(field 1) andintents(field 2) are populated —intentsis a redundant singleton.- proof lookup → only
intents(field 2) is populated —intentis left unset.Clients cannot uniformly read one field; they must track which request variant was used to decide whether to read
intentorintents. Consider one of these cleaner alternatives:Option A — clean separation (no redundancy):
Populate onlyintentfor txid lookups, onlyintentsfor proof lookups.Option B —
intentsas the canonical field (always safe to read):
Always populateintents; additionally populateintentfor txid lookups for backward compatibility.-// If response coming from GetIntentRequest call by txid, both fields here will be populated. -// If response coming from GetIntentRequest call by intent, only intents will be populated. +// If the request used the txid filter, intent (field 1) is populated (backward-compat). +// If the request used the intent filter, only intents (field 2) is populated. +// Prefer reading intents for new code; intent is kept for backward compatibility only. message GetIntentResponse { Intent intent = 1; repeated Intent intents = 2; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-spec/protobuf/ark/v1/service.proto` around lines 333 - 337, The GetIntentResponse currently has asymmetric field population between intent and intents; make intents the canonical, always-populated field and keep intent only for txid-backwards-compatibility: update the GetIntentResponse contract so servers always populate the repeated field intents and, when handling a txid GetIntentRequest, also set the single intent field to the first element (preserving existing behavior); update the message comment to document this rule and adjust any code path that constructs GetIntentResponse (handlers that build responses for txid or proof lookups) to always fill intents and optionally set intent for txid cases, plus add/update tests to assert the new invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api-spec/protobuf/ark/v1/service.proto`:
- Around line 165-169: The GET binding on rpc GetIntent causes the intent oneof
(PSBT proof) to be serialized into query params; add a non-breaking POST
alternative by adding an additional_bindings block to the existing option
(meshapi.gateway.http) on GetIntent: keep the existing get: "/v1/intent" and add
an additional_bindings entry with post: "/v1/intent" and body: "*" (or body:
"intent" if you want to limit to that field) so the PSBT/proof can be sent in
the request body while preserving the current GET behavior.
---
Nitpick comments:
In `@api-spec/protobuf/ark/v1/service.proto`:
- Around line 327-330: The oneof block for `filter` has inconsistent
indentation: its fields `txid` and `intent` are indented differently than other
oneof blocks; update the `oneof filter` block so that the fields `txid = 1;` and
`Intent intent = 2;` use 4-space indentation (matching other oneof blocks) and
ensure the closing `}` aligns consistently with those blocks.
- Around line 333-337: The GetIntentResponse currently has asymmetric field
population between intent and intents; make intents the canonical,
always-populated field and keep intent only for txid-backwards-compatibility:
update the GetIntentResponse contract so servers always populate the repeated
field intents and, when handling a txid GetIntentRequest, also set the single
intent field to the first element (preserving existing behavior); update the
message comment to document this rule and adjust any code path that constructs
GetIntentResponse (handlers that build responses for txid or proof lookups) to
always fill intents and optionally set intent for txid cases, plus add/update
tests to assert the new invariant.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/ark/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (2)
api-spec/openapi/swagger/ark/v1/service.openapi.jsonapi-spec/protobuf/ark/v1/service.proto
🚧 Files skipped from review as they are similar to previous changes (1)
- api-spec/openapi/swagger/ark/v1/service.openapi.json
…roof # Conflicts: # internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go
internal/core/domain/round_repo.go
Outdated
| GetTxsWithTxids(ctx context.Context, txids []string) ([]string, error) | ||
| GetRoundsWithCommitmentTxids(ctx context.Context, txids []string) (map[string]any, error) | ||
| GetIntentByTxid(ctx context.Context, txid string) (*Intent, error) | ||
| GetIntentsByProof(ctx context.Context, proof string) ([]*Intent, error) |
There was a problem hiding this comment.
it seems like this is not needed?
internal/core/application/service.go
Outdated
| // verifyIntentProofAndFindMatches validates proof-of-ownership inputs, signs and | ||
| // verifies the proof, then returns all cached intents whose inputs overlap with | ||
| // the proof outpoints. | ||
| func (s *service) verifyIntentProofAndFindMatches( |
There was a problem hiding this comment.
please move this function to the bottom after all service's public methods
| @@ -0,0 +1 @@ | |||
| CREATE INDEX IF NOT EXISTS idx_intent_proof ON intent(proof); | |||
There was a problem hiding this comment.
we don't really need these indexes on pg and sqlite? is seems to me we never query intents by proof
There was a problem hiding this comment.
well not yet but i think the spirit of the issue implies clients will
pkg/ark-lib/intent/message_test.go
Outdated
| func TestGetIntentMessageDecode(t *testing.T) { | ||
| t.Run("valid", func(t *testing.T) { | ||
| var msg intent.GetIntentMessage | ||
| err := msg.Decode(`{"type":"get-intent","expire_at":1762862054}`) | ||
| require.NoError(t, err) | ||
| require.Equal(t, intent.IntentMessageTypeGetIntent, msg.Type) | ||
| require.Equal(t, int64(1762862054), msg.ExpireAt) | ||
| }) | ||
|
|
||
| t.Run("wrong_type", func(t *testing.T) { | ||
| var msg intent.GetIntentMessage | ||
| err := msg.Decode(`{"type":"delete","expire_at":1762862054}`) | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "invalid intent message type") | ||
| }) | ||
|
|
||
| t.Run("invalid_json", func(t *testing.T) { | ||
| var msg intent.GetIntentMessage | ||
| err := msg.Decode(`not json`) | ||
| require.Error(t, err) | ||
| }) | ||
| } | ||
|
|
||
| func TestGetIntentMessageAccessors(t *testing.T) { | ||
| msg := intent.GetIntentMessage{ | ||
| BaseMessage: intent.BaseMessage{Type: intent.IntentMessageTypeGetIntent}, | ||
| ExpireAt: 1762862054, | ||
| } | ||
|
|
||
| require.Equal(t, int64(1762862054), msg.GetExpireAt()) | ||
| require.Equal(t, intent.BaseMessage{Type: intent.IntentMessageTypeGetIntent}, msg.GetBaseMessage()) | ||
| } | ||
|
|
||
| func TestDeleteMessageAccessors(t *testing.T) { | ||
| msg := intent.DeleteMessage{ | ||
| BaseMessage: intent.BaseMessage{Type: intent.IntentMessageTypeDelete}, | ||
| ExpireAt: 1762862054, | ||
| } | ||
|
|
||
| require.Equal(t, int64(1762862054), msg.GetExpireAt()) | ||
| require.Equal(t, intent.BaseMessage{Type: intent.IntentMessageTypeDelete}, msg.GetBaseMessage()) | ||
| } | ||
|
|
||
| func TestGetIntentMessageRoundtrip(t *testing.T) { | ||
| original := intent.GetIntentMessage{ | ||
| BaseMessage: intent.BaseMessage{Type: intent.IntentMessageTypeGetIntent}, | ||
| ExpireAt: 1762862054, | ||
| } | ||
|
|
||
| encoded, err := original.Encode() | ||
| require.NoError(t, err) | ||
|
|
||
| var decoded intent.GetIntentMessage | ||
| err = decoded.Decode(encoded) | ||
| require.NoError(t, err) | ||
| require.Equal(t, original, decoded) | ||
| } | ||
|
|
There was a problem hiding this comment.
let's remove these tests (but let's keep the fixtures you added), the purpose of TestIntentMessage is just to provide a standard json encoding
| "name": "get_intent_no_expiry", | ||
| "message": { | ||
| "type": "get-intent", | ||
| "expire_at": 0 | ||
| }, | ||
| "expected": "{\"type\":\"get-intent\",\"expire_at\":0}" | ||
| } |
There was a problem hiding this comment.
this test is not needed, the expected results are just JSONs to ensure the order of the fields, and stuff like that
3 participants
Closes #729
RPC support for fetching a registered intent by proof. Expanded upon the existing
GetIntentRequestwhere prior you could only pass atxid, now you can pass an intent instead. We then use the proof in the intent for proof of ownership of any one input of an intent (just like delete intent does). I actually moved the logic fromDeleteIntentsByProofininternal/core/application/service.gointo a function calledverifyIntentProofAndFindMatchessince this logic will also be used by the newGetIntentByProof.A new message type is added in
pkg/ark-lib/intent/message.go:IntentMessageTypeGetIntent IntentMessageType = "get-intent"The new
GetIntentByProofscan return more than one proof so we need the proto to support multiple proofs being returned.In order for the proto to remain backwards compatible, I changed from:
to:
Summary by CodeRabbit
New Features
Performance
Tests
Chores