feat: add GetPendingTx by txid #869
Conversation
This go-sdk at the moment, doesn't have `GetPendingTxByTxid` method currently, hence the function in the util_test.go file. Would add it in a separate PR and then remove this one.
WalkthroughAdds txid-based lookup to GetPendingTx (proto + OpenAPI), pending-tx-aware spent checks in submission, store methods to map outpoint→txid, a service method to fetch pending offchain tx by txid, new VTXO_PENDING_TX error metadata, and tests for txid lookup and edge cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as gRPC Handler
participant Service
participant Store as OffChainTxStore
alt identifier == Txid
Client->>Handler: GetPendingTx{identifier: Txid}
Handler->>Handler: validate txid
Handler->>Service: GetPendingOffchainTxByTxid(txid)
Service->>Store: lookup tx by txid (cache → repo)
alt found
Store-->>Service: AcceptedOffchainTx
Service-->>Handler: AcceptedOffchainTx
else not found
Store-->>Service: nil
Service-->>Handler: nil
end
Handler-->>Client: GetPendingTxResponse (single result or empty)
else identifier == Intent
Client->>Handler: GetPendingTx{identifier: Intent}
Handler->>Handler: validate/parse intent proof
Handler->>Service: GetPendingOffchainTxs(intent)
Service->>Store: query by ownership/intent
Store-->>Service: []AcceptedOffchainTx
Service-->>Handler: []AcceptedOffchainTx
Handler-->>Client: GetPendingTxResponse (results)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/infrastructure/live-store/inmemory/offchain_txs.go (1)
78-97: Consider maintaining a reverse index for O(1) lookup.The implementation correctly provides thread-safe outpoint-to-txid lookup and is consistent with existing patterns in this file. However, the current approach has O(n×m) complexity (iterating all offchain txs × checkpoint txs).
For better performance, consider maintaining a reverse index
map[string]string(outpoint.String() → txid) that's updated inAdd/Remove. This would provide O(1) lookup at the cost of slightly increased memory and code complexity.That said, for an in-memory store likely used in testing or small deployments, the current implementation is acceptable.
internal/infrastructure/live-store/redis/offchain_txs.go (1)
139-171: Silent error handling may mask data corruption.The method silently continues on JSON unmarshal errors (line 155-157) and PSBT parse errors (line 160-162), which differs from
Remove(lines 83-87) where PSBT parse errors are returned. This inconsistency could mask corrupted data in Redis.Additionally, if
SIsMemberreturnstruebut no matching transaction is found after iterating all transactions, the method returns"", nil. This could indicate a data inconsistency (orphaned entry in the set) that goes undetected.Consider:
- Logging errors instead of silently continuing, to aid debugging.
- Returning an error or logging a warning when the set indicates existence but no match is found.
outpointStr := outpoint.String() for txid, txStr := range txsMap { var offchainTx domain.OffchainTx if err := json.Unmarshal([]byte(txStr), &offchainTx); err != nil { + // Log error but continue to check other transactions continue } for _, tx := range offchainTx.CheckpointTxs { ptx, err := psbt.NewFromRawBytes(strings.NewReader(tx), true) if err != nil { + // Log error but continue to check other transactions continue } for _, in := range ptx.UnsignedTx.TxIn { if in.PreviousOutPoint.String() == outpointStr { return txid, nil } } } } + // Outpoint exists in set but not found in any transaction - potential data inconsistency return "", nil }internal/core/application/service.go (1)
1442-1468: Consider distinguishing "not found" from actual database errors.At lines 1453-1456, all errors from
GetOffchainTxresult in returningnil, nil, which treats database errors the same as "not found". Compare with line 481 where "not found" is explicitly checked:if err != nil && !strings.Contains(err.Error(), "not found") { return nil, errors.INTERNAL_ERROR.New(...) }This could hide database connectivity issues or other real errors.
if offchainTx == nil { offchainTx, err = s.repoManager.OffchainTxs().GetOffchainTx(ctx, txid) if err != nil { + if !strings.Contains(err.Error(), "not found") { + log.WithError(err).Error("failed to get offchain tx from repository") + return nil, errors.INTERNAL_ERROR.New("something went wrong"). + WithMetadata(map[string]any{"txid": txid}) + } return nil, nil } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (16)
api-spec/protobuf/gen/ark/v1/admin.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/admin_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/indexer.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/ark/v1/indexer_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/service_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/signer_manager.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/signer_manager_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/types.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/wallet.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/wallet_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/signer/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/signer/v1/service_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (11)
api-spec/openapi/swagger/ark/v1/service.openapi.json(1 hunks)api-spec/protobuf/ark/v1/service.proto(1 hunks)internal/core/application/service.go(3 hunks)internal/core/application/types.go(1 hunks)internal/core/ports/live_store.go(1 hunks)internal/infrastructure/live-store/inmemory/offchain_txs.go(1 hunks)internal/infrastructure/live-store/redis/offchain_txs.go(1 hunks)internal/interface/grpc/handlers/arkservice.go(1 hunks)internal/test/e2e/e2e_test.go(2 hunks)internal/test/e2e/utils_test.go(1 hunks)pkg/errors/errors.go(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/test/e2e/e2e_test.go
🧬 Code graph analysis (6)
internal/infrastructure/live-store/inmemory/offchain_txs.go (1)
internal/core/application/types.go (1)
Outpoint(184-184)
internal/core/ports/live_store.go (2)
internal/core/application/types.go (1)
Outpoint(184-184)internal/core/domain/vtxo.go (1)
Outpoint(15-18)
internal/infrastructure/live-store/redis/offchain_txs.go (2)
internal/core/application/types.go (1)
Outpoint(184-184)internal/core/domain/offchain_tx.go (1)
OffchainTx(35-48)
internal/core/application/service.go (2)
internal/core/application/types.go (2)
Outpoint(184-184)AcceptedOffchainTx(16-20)pkg/errors/errors.go (2)
VTXO_PENDING_TX(342-346)VtxoPendingTxMetadata(337-340)
internal/core/application/types.go (1)
pkg/errors/errors.go (1)
Error(39-46)
internal/interface/grpc/handlers/arkservice.go (5)
internal/core/application/types.go (1)
AcceptedOffchainTx(16-20)api-spec/protobuf/gen/ark/v1/service.pb.go (4)
GetPendingTxRequest_Txid(1399-1401)GetPendingTxRequest_Txid(1405-1405)GetPendingTxRequest_Intent(1395-1397)GetPendingTxRequest_Intent(1403-1403)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
Intent(441-447)Intent(460-460)Intent(475-477)internal/infrastructure/db/postgres/sqlc/queries/models.go (1)
Intent(33-38)internal/infrastructure/db/sqlite/sqlc/queries/models.go (1)
Intent(31-36)
🔇 Additional comments (13)
api-spec/openapi/swagger/ark/v1/service.openapi.json (1)
820-823: LGTM!The addition of the
txidproperty toGetPendingTxRequestcorrectly extends the REST API surface to support txid-based pending transaction retrieval, consistent with the protobuf oneof variant.api-spec/protobuf/ark/v1/service.proto (1)
264-264: LGTM!The addition of
txidas a oneof variant correctly extends GetPendingTxRequest to support transaction ID-based lookup while maintaining mutual exclusivity with Intent-based lookup.internal/core/ports/live_store.go (1)
54-54: LGTM!The addition of
GetTxidByOutpointto theOffChainTxStoreinterface correctly enables reverse lookup from outpoint to transaction ID, supporting the pending transaction detection flow.internal/core/application/types.go (1)
45-48: LGTM!The addition of
GetPendingOffchainTxByTxidto theServiceinterface correctly extends the service layer to support txid-based pending transaction retrieval, with a signature consistent with existing service methods.internal/test/e2e/utils_test.go (2)
785-793: LGTM!The helper types
pendingTxResponseandpendingTxare well-defined with appropriate JSON tags for HTTP-based testing of the GetPendingTx endpoint.
795-824: LGTM!The
getPendingTxByTxidhelper function is well-implemented with comprehensive error handling for all stages: request creation, execution, response reading, status validation, and JSON decoding. The function properly uses error wrapping and includes informative error messages.pkg/errors/errors.go (2)
337-340: LGTM!The
VtxoPendingTxMetadatastruct appropriately captures the necessary context (vtxo outpoint and pending txid) for pending transaction errors, with proper JSON tags for serialization.
342-346: LGTM!The
VTXO_PENDING_TXerror code is well-defined withgrpccodes.FailedPrecondition, which is the appropriate gRPC status code for indicating a resource is locked by another operation. This clearly distinguishes locked-by-pending-tx cases from already-spent cases.internal/test/e2e/e2e_test.go (2)
3987-4149: LGTM!The test comprehensively validates GetPendingTx by txid functionality:
- Retrieval of submitted pending transaction
- Empty response for non-existent txid
- Error for empty txid
- Verification that finalized transactions are no longer pending
The test is well-structured with proper setup, execution, and cleanup.
4151-4263: LGTM!The test effectively validates the VTXO locking behavior when a VTXO is locked by a pending transaction. Allowing either "pending" or "duplicated" in the error message is appropriate, as attempting to submit the same transaction twice could reasonably produce either error depending on internal timing and caching.
internal/interface/grpc/handlers/arkservice.go (1)
329-360: LGTM! Clean type-switch pattern for identifier handling.The implementation correctly handles both
TxidandIntentidentifier types with proper validation. The default case provides a safety net for unknown identifier types. The error handling is appropriate for each branch.internal/core/application/service.go (2)
532-547: LGTM! Proper pending transaction detection with descriptive error.The check correctly identifies VTXOs locked by pending transactions and returns the new
VTXO_PENDING_TXerror with appropriate metadata containing both the VTXO outpoint and the pending transaction ID. This provides clients with actionable information.
1087-1105: LGTM! Proper double-check pattern under mutex.The re-check after acquiring the mutex correctly prevents race conditions between concurrent offchain transaction submissions. The comment clearly explains the rationale.
| SignedCheckpointTxs []string `json:"signedCheckpointTxs"` | ||
| } | ||
|
|
||
| func getPendingTxByTxid(httpClient *http.Client, txid string) (*pendingTxResponse, error) { |
There was a problem hiding this comment.
we're gonna open pr on go-sdk for u so that we can drop this. Will ping u as soon as it's done
altafan
left a comment
altafan
left a comment
There was a problem hiding this comment.
@Chigala Thank you for the contrib! 💪
Can you please add unit tests to internal/infrastructure/live-store/live_store_test.go for the newly introduced OffChainTxStore.GetTxidByOutpoint(...) api?
Also, please run locally make lint to lint the code so that ci moves on and runs unit tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/test/e2e/e2e_test.go (1)
4100-4100: Consider a retry loop for improved robustness.The 2-second sleep is probably sufficient for ensuring the transaction is stored, but a retry loop with timeout would be more robust in high-load CI environments. However, this is acceptable for e2e tests where explicit waits are common.
Optional: Replace sleep with retry pattern
- // Wait a bit longer to ensure the transaction is stored - time.Sleep(2 * time.Second) + // Poll for pending tx with timeout + var pendingTxResp *PendingTxResponse + for i := 0; i < 10; i++ { + pendingTxResp, err = getPendingTxByTxid(httpClient, txid) + require.NoError(t, err) + if len(pendingTxResp.PendingTxs) > 0 { + break + } + time.Sleep(200 * time.Millisecond) + } + require.NotNil(t, pendingTxResp) + require.Len(t, pendingTxResp.PendingTxs, 1)internal/core/application/service.go (1)
1442-1468: Consider logging repository errors for better observability.At line 1455, the method returns
nil, nilwhenGetOffchainTxfails, silently swallowing the error. While returningnilfor "not found" is acceptable, other errors (e.g., database connection issues) should be logged for debugging purposes.🔎 Apply this diff to add error logging:
if offchainTx == nil { offchainTx, err = s.repoManager.OffchainTxs().GetOffchainTx(ctx, txid) if err != nil { + if !strings.Contains(err.Error(), "not found") { + log.WithError(err).Error("failed to get offchain tx from repository") + } return nil, nil } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/core/application/service.go(3 hunks)internal/infrastructure/live-store/live_store_test.go(1 hunks)internal/interface/grpc/handlers/arkservice.go(1 hunks)internal/test/e2e/e2e_test.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/core/application/service.gointernal/test/e2e/e2e_test.go
🧬 Code graph analysis (4)
internal/infrastructure/live-store/live_store_test.go (2)
internal/core/application/types.go (2)
Outpoint(184-184)VOut(138-138)internal/core/domain/vtxo.go (1)
Outpoint(15-18)
internal/core/application/service.go (2)
internal/core/application/types.go (2)
Outpoint(184-184)AcceptedOffchainTx(16-20)pkg/errors/errors.go (2)
VTXO_PENDING_TX(342-346)VtxoPendingTxMetadata(337-340)
internal/interface/grpc/handlers/arkservice.go (3)
internal/core/application/types.go (1)
AcceptedOffchainTx(16-20)api-spec/protobuf/gen/ark/v1/service.pb.go (4)
GetPendingTxRequest_Txid(1399-1401)GetPendingTxRequest_Txid(1405-1405)GetPendingTxRequest_Intent(1395-1397)GetPendingTxRequest_Intent(1403-1403)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
Intent(441-447)Intent(460-460)Intent(475-477)
internal/test/e2e/e2e_test.go (6)
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 (2)
Outpoint(184-184)VOut(138-138)pkg/errors/errors.go (1)
Error(39-46)
🔇 Additional comments (5)
internal/test/e2e/e2e_test.go (2)
3987-4309: LGTM! Well-structured test coverage for txid-based pending transaction retrieval.The test structure follows best practices with clear separation of valid and invalid scenarios. The coverage includes:
- Happy path: retrieving and finalizing a pending tx by txid
- Edge cases: non-existent txid returns empty response
- Error cases: missing txid validation and double-spend detection
The test aligns with the past review feedback regarding subtest organization.
4160-4307: Excellent test for double-spend detection with pending transactions.This test properly validates that the server detects when a VTXO is already locked by a pending transaction and returns the appropriate
VTXO_PENDING_TXerror. The approach of using a different pkscript ensures the transactions have different txids, making this a robust test of the double-spend detection logic.internal/infrastructure/live-store/live_store_test.go (1)
337-346: LGTM! Clear and focused test coverage for GetTxidByOutpoint.The tests properly verify both the positive case (existing outpoint returns the txid) and the negative case (non-existent outpoint returns empty string). The assertions match the expected behavior documented in the interface.
internal/interface/grpc/handlers/arkservice.go (1)
329-360: LGTM! Clean implementation of dual identifier support.The handler properly supports both
TxidandIntentidentifiers with:
- Appropriate validation for each identifier type (empty txid check, nil intent check)
- Consistent error handling using
status.Errorwithcodes.InvalidArgument- Nil-check on Line 340 before wrapping the single result
- Default case to reject unsupported identifier types
The implementation follows established patterns in the codebase and integrates cleanly with the existing flow.
internal/core/application/service.go (1)
532-547: LGTM! Pending-tx aware check implemented correctly.The replacement of the spent-input check with
GetTxidByOutpointlookup properly detects when a VTXO is locked by another pending transaction. The error message and metadata are appropriate for client-side handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/infrastructure/live-store/redis/offchain_txs.go (2)
139-174: Consider the performance implications of fetching all pending transactions.The method calls
HGetAllto retrieve all offchain transactions and then iterates through them to find a matching outpoint. While this implementation is functionally correct, it could become a performance bottleneck if the number of concurrent pending transactions grows large, as each lookup is O(n) where n is the number of pending transactions.Since Redis doesn't natively support reverse indexes, the current approach is reasonable for moderate transaction volumes. However, if performance becomes a concern, consider maintaining a separate Redis hash mapping
outpoint → txidthat is kept in sync with the main transaction storage.</review_comment_end>
158-165: Silent error handling may hide data corruption issues.The method silently continues when encountering unmarshal errors (line 159) or PSBT parsing errors (line 164). While this enables graceful degradation when some entries are malformed, it could mask data integrity issues in the store.
Consider logging these errors at a warning or debug level to aid troubleshooting without breaking the lookup flow.
</review_comment_end>
internal/test/e2e/utils_test.go (1)
795-829: UseserverUrlconstant instead of hardcoded URL for consistency.The function hardcodes "http://localhost:7070/v1/tx/pending", while other functions in this file use the
serverUrlconstant defined at line 44. For consistency and maintainability, construct the URL using the constant.Note: As indicated in the previous review comment, this helper is temporary and will be replaced by a proper SDK method in a follow-up PR.
🔎 Suggested fix for URL consistency:
func getPendingTxByTxid(httpClient *http.Client, txid string) (*pendingTxResponse, error) { reqBody := bytes.NewReader([]byte(fmt.Sprintf(`{"txid": "%s"}`, txid))) - req, err := http.NewRequest("POST", "http://localhost:7070/v1/tx/pending", reqBody) + req, err := http.NewRequest("POST", fmt.Sprintf("http://%s/v1/tx/pending", serverUrl), reqBody) if err != nil { return nil, fmt.Errorf("failed to prepare request: %w", err) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/infrastructure/live-store/inmemory/offchain_txs.go(1 hunks)internal/infrastructure/live-store/live_store_test.go(1 hunks)internal/infrastructure/live-store/redis/offchain_txs.go(1 hunks)internal/test/e2e/utils_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/infrastructure/live-store/live_store_test.go
- internal/infrastructure/live-store/inmemory/offchain_txs.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/infrastructure/live-store/redis/offchain_txs.go (2)
internal/core/application/types.go (1)
Outpoint(184-184)internal/core/domain/offchain_tx.go (1)
OffchainTx(35-48)
🔇 Additional comments (1)
internal/test/e2e/utils_test.go (1)
785-793: LGTM - Clean data structures for pending transaction responses.The
pendingTxResponseandpendingTxtypes appropriately model the API response structure and align with the new txid-based lookup functionality.</review_comment_end>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/core/application/service.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/core/application/service.go (3)
internal/core/application/types.go (2)
Outpoint(184-184)AcceptedOffchainTx(16-20)internal/core/domain/vtxo.go (1)
Outpoint(15-18)pkg/errors/errors.go (2)
VTXO_PENDING_TX(342-346)VtxoPendingTxMetadata(337-340)
🔇 Additional comments (2)
internal/core/application/service.go (2)
532-547: LGTM! Pending-tx check correctly implemented.The replacement of the simple spent-check with
GetTxidByOutpointcorrectly identifies VTXOs locked by pending transactions. The error message follows the generic format suggested in past reviews, and the metadata provides both the VTXO outpoint and pending txid for client-side handling.
1087-1106: LGTM! Race condition check correctly implemented with consistent error message.The post-lock check using
GetTxidByOutpointprevents race conditions between concurrent offchain transaction submissions. The error message now matches the earlier check at line 542, addressing the past review feedback.
| func (s *service) GetPendingOffchainTxByTxid( | ||
| ctx context.Context, txid string, | ||
| ) (*AcceptedOffchainTx, errors.Error) { | ||
| offchainTx, err := s.cache.OffchainTxs().Get(ctx, txid) | ||
| if err != nil { | ||
| log.WithError(err).Error("failed to get offchain tx from cache") | ||
| return nil, errors.INTERNAL_ERROR.New("something went wrong"). | ||
| WithMetadata(map[string]any{"txid": txid}) | ||
| } | ||
|
|
||
| if offchainTx == nil { | ||
| offchainTx, err = s.repoManager.OffchainTxs().GetOffchainTx(ctx, txid) | ||
| if err != nil { | ||
| return nil, nil | ||
| } | ||
| } | ||
|
|
||
| if !offchainTx.IsAccepted() { | ||
| return nil, nil | ||
| } | ||
|
|
||
| return &AcceptedOffchainTx{ | ||
| TxId: offchainTx.ArkTxid, | ||
| FinalArkTx: offchainTx.ArkTx, | ||
| SignedCheckpointTxs: offchainTx.CheckpointTxsList(), | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Critical: Potential nil pointer dereference and inconsistent error handling.
Two issues need attention in GetPendingOffchainTxByTxid:
-
Nil pointer dereference risk (line 1459): If
GetOffchainTxreturns(nil, nil)or ifoffchainTxis stillnilafter error handling at line 1454, callingoffchainTx.IsAccepted()at line 1459 will panic. -
Inconsistent error handling (line 1454): The method returns
(nil, nil)for all repository errors without logging or distinguishing between "not found" and actual database errors. Compare with the pattern at line 481 which checks for "not found" specifically, or line 1139-1143 which returnsTX_NOT_FOUNDerror.
🔎 Apply this diff to fix both issues:
if offchainTx == nil {
offchainTx, err = s.repoManager.OffchainTxs().GetOffchainTx(ctx, txid)
- if err != nil {
+ if err != nil && !strings.Contains(err.Error(), "not found") {
+ log.WithError(err).Error("failed to get offchain tx from repository")
+ return nil, errors.INTERNAL_ERROR.New("something went wrong").
+ WithMetadata(map[string]any{"txid": txid})
+ }
+ if offchainTx == nil {
return nil, nil
}
}
if !offchainTx.IsAccepted() {
return nil, nil
}🤖 Prompt for AI Agents
internal/core/application/service.go around lines 1442 to 1468: current code can
dereference offchainTx and swallows repo errors; after calling
s.repoManager.OffchainTxs().GetOffchainTx, first check and handle errors (log
the error and return an INTERNAL_ERROR with txid metadata for unexpected DB
errors, or map repository "not found" into a specific TX_NOT_FOUND error as used
elsewhere), then check if offchainTx == nil and return TX_NOT_FOUND (with txid
metadata) instead of proceeding; only after those checks call
offchainTx.IsAccepted() to avoid nil pointer dereference.
2 participants
This PR extends the
GetPendingTxRPC to allow fetching pending transactions bytxidin addition to the existing Intent mechanism. It also introduces a dedicated structured error for when a VTXO is locked because of a pending transaction. I followed the same patterns used in this PR. I also added new test suites to test this here.A good follow-up task to this would be adding a
GetPendingTxByTxidmethod to the go-sdk. This would remove thegetPendingTxByTxidfunction I have at the moment in theutils_test.gofile.Closes #832
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.