Implement GetPendingTx RPC by intent #832
Conversation
|
Warning Rate limit exceeded@louisinger has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughRefactors offchain submission to return an AcceptedOffchainTx object, adds GetPendingOffchainTxs service method and gRPC GetPendingTx handler, introduces a GetPendingTx intent message and parser, adds domain accessor and e2e test, and updates two dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as gRPC Client
participant Handler as arkservice Handler
participant Service as Application Service
participant Domain as Domain Model
rect rgb(240,248,255)
note right of Handler: GetPendingTx flow
Client->>Handler: GetPendingTx(intent)
Handler->>Handler: parseGetPendingTxIntent()
Handler->>Service: GetPendingOffchainTxs(proof, message)
Service->>Domain: Query pending offchain txs
Domain-->>Service: []AcceptedOffchainTx
Service-->>Handler: []AcceptedOffchainTx, error
Handler-->>Client: PendingTx list
end
rect rgb(255,250,240)
note right of Handler: SubmitTx (refactored)
Client->>Handler: SubmitTx(unsignedCheckpointTxs, signedArkTx)
Handler->>Service: SubmitOffchainTx(...)
Service->>Domain: Process transaction, produce accepted result
Domain-->>Service: AcceptedOffchainTx{TxId, FinalArkTx, SignedCheckpointTxs}
Service-->>Handler: *AcceptedOffchainTx, error
Handler-->>Client: Response {TxId, FinalArkTx, SignedCheckpointTxs}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/core/domain/offchain_tx.go (1)
219-225: Consider deduplication for consistency.The
CommitmentTxidsList()method (lines 207-217) deduplicates values from the map before returning them, butCheckpointTxsList()does not. IfCheckpointTxsmap values can have duplicates (multiple keys pointing to the same checkpoint transaction ID), this could lead to inconsistent behavior.If deduplication is needed, apply this diff:
func (s *OffchainTx) CheckpointTxsList() []string { - list := make([]string, 0, len(s.CheckpointTxs)) - for _, tx := range s.CheckpointTxs { - list = append(list, tx) + indexedList := make(map[string]struct{}) + for _, txid := range s.CheckpointTxs { + indexedList[txid] = struct{}{} + } + list := make([]string, 0, len(indexedList)) + for txid := range indexedList { + list = append(list, txid) } return list }internal/interface/grpc/handlers/arkservice.go (1)
306-329: LGTM with optional defensive check.The
GetPendingTximplementation follows established patterns and correctly maps service results to proto messages. The code is clear and handles errors appropriately.Consider adding a defensive nil check for robustness:
pendingTxs, err := h.svc.GetPendingOffchainTxs(ctx, *proof, *message) if err != nil { return nil, err } + if pendingTxs == nil { + pendingTxs = []application.AcceptedOffchainTx{} + } pendingTxsProto := make([]*arkv1.PendingTx, 0, len(pendingTxs))internal/core/application/service.go (1)
1367-1407: Consider optimizing the pending transaction retrieval as noted in the TODO.The current implementation iterates through all vtxos and fetches offchain transactions individually. The TODO comment at line 1371 suggests filtering vtxos where the associated ArkTxid outputs don't exist in the DB, which could significantly improve performance for users with many vtxos.
Consider implementing the suggested optimization in a follow-up if this endpoint is expected to handle users with large numbers of vtxos.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.mod(1 hunks)internal/core/application/service.go(46 hunks)internal/core/application/types.go(2 hunks)internal/core/domain/offchain_tx.go(1 hunks)internal/interface/grpc/handlers/arkservice.go(2 hunks)internal/interface/grpc/handlers/parser.go(1 hunks)internal/test/e2e/e2e_test.go(2 hunks)pkg/ark-lib/intent/message.go(2 hunks)pkg/ark-lib/intent/message_test.go(1 hunks)pkg/ark-lib/intent/testdata/message_fixtures.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T10:58:41.042Z
Learnt from: louisinger
Repo: arkade-os/arkd PR: 691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.
Applied to files:
internal/core/domain/offchain_tx.gointernal/interface/grpc/handlers/arkservice.gointernal/test/e2e/e2e_test.gointernal/core/application/types.gointernal/core/application/service.go
🧬 Code graph analysis (6)
internal/interface/grpc/handlers/arkservice.go (2)
api-spec/protobuf/gen/ark/v1/service.pb.go (6)
SubmitTxResponse(1089-1096)SubmitTxResponse(1109-1109)SubmitTxResponse(1124-1126)GetPendingTxResponse(1281-1286)GetPendingTxResponse(1299-1299)GetPendingTxResponse(1314-1316)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
PendingTx(689-696)PendingTx(709-709)PendingTx(724-726)
pkg/ark-lib/intent/message_test.go (1)
pkg/ark-lib/intent/message.go (2)
IntentMessageTypeGetPendingTx(15-15)GetPendingTxMessage(88-93)
internal/interface/grpc/handlers/parser.go (3)
api-spec/protobuf/gen/ark/v1/types.pb.go (3)
Intent(441-447)Intent(460-460)Intent(475-477)pkg/ark-lib/intent/proof.go (1)
Proof(39-41)pkg/ark-lib/intent/message.go (1)
GetPendingTxMessage(88-93)
internal/test/e2e/e2e_test.go (4)
pkg/ark-lib/script/vtxo_script.go (1)
ParseVtxoScript(37-53)pkg/ark-lib/txutils/taptree.go (1)
TapTree(14-14)pkg/ark-lib/encoding.go (2)
DecodeAddressV0(55-98)Address(13-18)pkg/ark-lib/offchain/tx.go (2)
BuildTxs(31-73)VtxoInput(20-28)
internal/core/application/types.go (3)
pkg/errors/errors.go (1)
Error(39-46)pkg/ark-lib/intent/proof.go (1)
Proof(39-41)pkg/ark-lib/intent/message.go (1)
GetPendingTxMessage(88-93)
internal/core/application/service.go (4)
internal/core/application/types.go (2)
AcceptedOffchainTx(16-20)Outpoint(177-177)pkg/ark-lib/intent/proof.go (3)
New(134-158)Proof(39-41)Verify(51-126)internal/core/domain/vtxo.go (2)
Outpoint(15-18)Vtxo(38-53)pkg/ark-lib/intent/message.go (2)
GetPendingTxMessage(88-93)BaseMessage(20-22)
🔇 Additional comments (11)
pkg/ark-lib/intent/message.go (1)
13-15: LGTM!The new
GetPendingTxMessagetype follows the established pattern for intent messages consistently. The implementation correctly validates the message type during decoding and provides symmetric encode/decode operations.Also applies to: 88-113
pkg/ark-lib/intent/testdata/message_fixtures.json (1)
31-39: LGTM!The new test fixture for
pending_txis consistent with existing fixtures and properly represents the expected JSON encoding for aGetPendingTxMessage.pkg/ark-lib/intent/message_test.go (1)
69-73: LGTM!The test case properly handles the new
GetPendingTxMessagetype and follows the established pattern for message decoding tests.internal/interface/grpc/handlers/arkservice.go (1)
272-283: LGTM!The refactored
SubmitTxhandler correctly uses the new unifiedAcceptedOffchainTxreturn type and properly populates the response fields.internal/interface/grpc/handlers/parser.go (1)
69-86: LGTM!The
parseGetPendingTxIntentfunction is well-implemented and follows the established pattern for intent parsing. Error messages are clear and specific.internal/test/e2e/e2e_test.go (1)
690-690: LGTM!Using
t.Context()instead ofcontext.Background()ensures proper test lifecycle management and allows the test to be cancelled if needed.go.mod (1)
24-24: The verification reveals significant concerns. Let me search for additional context about the pending-txs branch and this PR.Based on the verification results, I've identified critical dependency issues that need to be addressed:
Both dependency versions reference unverified or unstable commits.
The verification reveals:
- arkade-os/arkd/pkg/ark-lib (line 24): Commit
494b4d36e7accannot be found in the repository—this is non-existent or inaccessible- arkade-os/go-sdk (line 30): Commit
704575432df0exists only on thepending-txsfeature branch and PR #60, not on the main branchUsing commits from unmerged feature branches or non-existent commits in
go.modcreates significant risks: the changes are not stable, may be abandoned, could introduce breaking changes, and won't be reproducible if branches are deleted or force-pushed.Before merging, ensure:
- The ark-lib commit exists in the repository (verify the correct commit hash or use a released version)
- The go-sdk changes are merged to main or switch to a stable, released version
- Consider whether development/testing versions belong in this PR or if stable releases should be used
internal/core/application/types.go (2)
16-20: LGTM! Clean refactoring of return values.The
AcceptedOffchainTxstruct effectively consolidates the multiple return values fromSubmitOffchainTxinto a single, cohesive type. This improves API clarity and makes the code more maintainable.
33-41: LGTM! Interface changes align well with the new workflow.The updated
SubmitOffchainTxsignature and newGetPendingOffchainTxsmethod properly support the pending transaction retrieval feature. The return types are appropriate for their respective use cases.internal/core/application/service.go (2)
1072-1081: LGTM! Return value refactoring is correctly implemented.The construction of
AcceptedOffchainTxproperly consolidates the previously separate return values into a single struct. All fields are correctly populated and the refactoring maintains backward compatibility in terms of the data returned.
1400-1404: The verification confirms that theCheckpointTxsList()method exists on theOffchainTxtype with the correct signature and implementation. The method properly returns[]stringas expected.
No issues found—the CheckpointTxsList method is properly implemented.
The method is correctly defined at
internal/core/domain/offchain_tx.go:219with the signaturefunc (s *OffchainTx) CheckpointTxsList() []string, and the implementation properly builds and returns a string slice from the checkpoint transactions. The call atservice.go:1400-1404is valid.
| offchainTx, err := offchainTxRepo.GetOffchainTx(ctx, vtxo.ArkTxid) | ||
| if err != nil { | ||
| log.WithError(err).Errorf("failed to get offchain tx %s", vtxo.ArkTxid) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Consider the implications of lenient error handling.
When fetching an offchain transaction fails, the error is logged but execution continues rather than returning an error to the caller. This could hide database issues or data inconsistencies, potentially causing the user to receive an incomplete list of pending transactions without knowing that some were skipped.
If this lenient behavior is intentional for resilience, consider adding a comment explaining the rationale. Otherwise, consider returning an error or collecting errors to return alongside the results.
🤖 Prompt for AI Agents
internal/core/application/service.go lines 1388-1392: the code logs an error
when offchainTxRepo.GetOffchainTx fails and then continues, which can silently
drop data; either propagate the error to the caller or aggregate errors to
return with the result, or if continuing is intentional add a clear comment
explaining the resilience rationale. Update the code to (a) return the wrapped
error immediately (or collect errors into a []error and return them alongside
the response), and (b) if you choose to continue on errors, replace the silent
continue with a short explanatory comment describing why skipping is safe and
what invariants ensure correctness.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/interface/grpc/handlers/arkservice.go (1)
309-316: Consider simplifying redundant null checks.The two null checks are somewhat redundant since
intentis currently the only field in theidentifieroneof. Ifidentifieris nil, thenintentwill also be nil.However, if you plan to add more identifier types to the oneof in the future, this defensive validation makes sense and can remain as-is.
If no additional identifier types are planned, you could simplify to:
- if req.GetIdentifier() == nil { - return nil, status.Error(codes.InvalidArgument, "missing identifier") - } - intent := req.GetIntent() if intent == nil { return nil, status.Error(codes.InvalidArgument, "missing intent") }
📜 Review details
Configuration used: CodeRabbit 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/protobuf/ark/v1/service.proto(1 hunks)internal/interface/grpc/handlers/arkservice.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T10:58:41.042Z
Learnt from: louisinger
Repo: arkade-os/arkd PR: 691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.
Applied to files:
internal/interface/grpc/handlers/arkservice.go
🧬 Code graph analysis (1)
internal/interface/grpc/handlers/arkservice.go (2)
api-spec/protobuf/gen/ark/v1/service.pb.go (6)
SubmitTxResponse(1089-1096)SubmitTxResponse(1109-1109)SubmitTxResponse(1124-1126)GetPendingTxResponse(1303-1308)GetPendingTxResponse(1321-1321)GetPendingTxResponse(1336-1338)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
PendingTx(689-696)PendingTx(709-709)PendingTx(724-726)
🪛 GitHub Actions: API Spec Changes
api-spec/protobuf/ark/v1/service.proto
[error] 246-246: buf breaking --against "https://github.com/arkade-os/arkd.git#branch=master,subdir=api-spec/protobuf" failed: Field "1" with name "intent" on message "GetPendingTxRequest" moved from outside to inside a oneof. (Process completed with exit code 100.)
⏰ 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: unit tests
- GitHub Check: Build and Scan
- GitHub Check: integration tests
🔇 Additional comments (2)
internal/interface/grpc/handlers/arkservice.go (2)
272-283: LGTM!The refactoring to use the
AcceptedOffchainTxobject returned bySubmitOffchainTxis clean and correctly maps the response fields.
328-337: LGTM!The mapping logic correctly transforms the service layer's
AcceptedOffchainTxobjects to protobufPendingTxmessages. Pre-allocating the slice with the correct capacity is good practice.
| oneof identifier { | ||
| Intent intent = 1; | ||
| } |
There was a problem hiding this comment.
Breaking protobuf change will break existing clients.
Moving the intent field from outside to inside a oneof is a breaking change that will cause compilation errors for existing clients using the old generated code. The generated accessor methods change, making this incompatible with the previous API.
If you intend to add additional identifier types in the future (e.g., string txid = 2;), then the oneof makes sense architecturally, but you'll need to:
- Communicate this breaking change to clients
- Bump the API version or provide a migration path
- Update client libraries
If no additional identifiers are planned, consider removing the oneof wrapper to avoid the breaking change.
🧰 Tools
🪛 GitHub Actions: API Spec Changes
[error] 246-246: buf breaking --against "https://github.com/arkade-os/arkd.git#branch=master,subdir=api-spec/protobuf" failed: Field "1" with name "intent" on message "GetPendingTxRequest" moved from outside to inside a oneof. (Process completed with exit code 100.)
🤖 Prompt for AI Agents
In api-spec/protobuf/ark/v1/service.proto around lines 245-247, wrapping the
existing Intent field inside a oneof is a breaking change; either revert the
change by moving "Intent intent = 1;" back out of the oneof to preserve existing
generated accessors, or if you must introduce the oneof for future identifier
types, treat it as a breaking API bump: move this change to a new v2 proto
package (or increment the service API version), update the protobuf
package/options and documentation, add migration notes for clients, and
coordinate releasing updated client libraries that consume the new generated
code.
This PR implements
GetPendingTxRPC, letting user to provider proof of ownership and get not finalized offchain transactions. It allows client to continue proceeding transaction if they sentSubmitTxrequest withoutFinalizeTxone.it introduces a new intent type with message
get-pending-tx.It closes #830
@altafan @sekulicd @Kukks please review
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.