Fix handling err in defer func in OffchainTx apis #825

altafan · 2025-11-21T13:38:20Z

Summary by CodeRabbit

Refactor
- Updated transaction submission and finalization interfaces, including return-value naming and assembly of signed transactions.
- Consolidated error propagation to a single structured error path and enhanced logging (now includes transaction IDs on save failures).
- Reworked internal checkpoint handling to build final signed lists at return time for clearer outcomes.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-11-21T13:38:38Z

Walkthrough

Updated SubmitOffchainTx and FinalizeOffchainTx in internal/core/application/service.go: both signatures now use named return structErr errors.Error, internal variable renames (e.g., fullySignedArkTx) and error propagation switched to structErr; signed checkpoint TXs are assembled via a map-to-slice conversion at return.

Changes

Cohort / File(s)	Summary
SubmitOffchainTx refactor `internal/core/application/service.go`	Signature changed to return named values `(signedCheckpointTxs []string, finalArkTx, finalArkTxid string, structErr errors.Error)`. Internal renames: `finalArkTx` → `fullySignedArkTx`. PSBT parsing uses short declaration (`arkPtx`/`arkTx`). Deferred error handling and propagation switched to `structErr`. Signed checkpoint TXs are produced from an intermediate map into a slice at return. Logging/save error messages include txid; Accept call updated to use `fullySignedArkTx`.
FinalizeOffchainTx refactor `internal/core/application/service.go`	Signature changed to return `(structErr errors.Error)`. Ark PSBT parsing uses short declaration. Deferred save/error handling switched to use `structErr`. Error logging for finalization save includes txid. Several error return paths converted to assign/return `structErr` instead of local `err`.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Service
  participant ArkLib as PSBT Parser
  participant DB

  Client->>Service: SubmitOffchainTx(unsignedCheckpointTxs, signedArkTx)
  Service->>ArkLib: Parse signedArkTx (short decl)
  ArkLib-->>Service: arkPtx / arkTx
  Service->>Service: assemble checkpoint map, sign/accept, set fullySignedArkTx
  Service->>DB: deferred save (tx metadata + fullySignedArkTx)
  DB-->>Service: save result / error
  Service-->>Client: return signedCheckpointTxs (from map), fullySignedArkTx, txid, structErr

sequenceDiagram
  participant Client
  participant Service
  participant ArkLib as PSBT Parser
  participant DB

  Client->>Service: FinalizeOffchainTx(txid, finalCheckpointTxs)
  Service->>ArkLib: Parse stored ark PSBT (short decl)
  ArkLib-->>Service: arkTx
  Service->>DB: save finalization (deferred)
  DB-->>Service: save result / error
  Service-->>Client: return structErr

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Check all error paths now consistently assign/return structErr.
Verify map-to-slice conversion of signed checkpoint TXs preserves required ordering/contents.
Confirm callers are compatible with the renamed/returned values (e.g., fullySignedArkTx / finalArkTx position).

Possibly related PRs

Concurrency RegisterIntent and SubmitTx tests and fix #824 — Modifies the same SubmitOffchainTx area in internal/core/application/service.go; likely overlaps with signature/flow changes.
Various stability improvements #682 — Also changes SubmitOffchainTx internals (checkpoint input/tapscript handling); strong code-level relation.
Replace log.Fatal by log.Error #817 — Related changes to error-handling and logging within these service methods.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the main change: fixing error handling in defer functions within OffchainTx APIs by switching from local err variables to structErr.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5f19b6e and 0db4008.

📒 Files selected for processing (1)

internal/core/application/service.go (6 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

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.

📚 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.go

🧬 Code graph analysis (1)

internal/core/application/service.go (2)

pkg/errors/errors.go (4)

Error (39-46)

INTERNAL_ERROR (208-208)

VTXO_ALREADY_SPENT (234-234)

VtxoMetadata (107-109)

internal/core/domain/offchain_tx_event.go (1)

OffchainTxTopic (3-3)

⏰ 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: Build and Scan
GitHub Check: unit tests
GitHub Check: integration tests

🔇 Additional comments (4)

internal/core/application/service.go (4)

414-436: LGTM: Proper error handling with named returns in defer

The use of named return structErr correctly enables the deferred function to capture and handle errors from any return path. This is the standard Go pattern for modifying return values in defer statements and properly addresses the error handling issue mentioned in the PR title.

The defer ensures that:

Any error is passed to offchainTx.Fail()

Events are always saved to the repository

Save failures are logged with the txid for debugging

1010-1010: Good: More descriptive variable naming

Renaming to fullySignedArkTx better conveys that this is the transaction after ASP signing has been applied, improving code readability.

1081-1101: LGTM: Consistent error handling pattern in FinalizeOffchainTx

The changes mirror the improvements in SubmitOffchainTx:

Named return structErr enables proper error handling in defer

changes variable properly scoped for defer to append to it

Improved logging includes txid for debugging

The pattern is consistent and correctly addresses the error handling issue.

1072-1076: No issues found - order-independent design is correct

The map-to-slice conversion produces non-deterministic ordering, but this is intentional and safe. The domain layer validates checkpoint transactions by count only (offchain_tx.go:107), and each transaction is independently identified by its txid from the PSBT itself. Clients parse the returned slice to extract transaction identifiers, not relying on position. The code is correct as-is.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

internal/core/application/service.go

altafan added 2 commits November 21, 2025 14:36

Fix handling err in defer func

c79e1cd

Lint

5f19b6e

louisinger approved these changes Nov 21, 2025

View reviewed changes

internal/core/application/service.go Outdated Show resolved Hide resolved

Fix

0db4008

altafan merged commit 7afe52a into arkade-os:master Nov 21, 2025
5 of 6 checks passed

This was referenced Nov 24, 2025

Implement GetPendingTx RPC by intent #832

Merged

FinalizeOffchainTx: add fallback to db to fetch offchain tx by id #834

Closed

Sanity checks on offchain tx flow #845

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix handling err in defer func in OffchainTx apis #825

Fix handling err in defer func in OffchainTx apis #825

Uh oh!

altafan commented Nov 21, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 21, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix handling err in defer func in OffchainTx apis #825

Fix handling err in defer func in OffchainTx apis #825

Uh oh!

Conversation

altafan commented Nov 21, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

altafan commented Nov 21, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 21, 2025 •

edited

Loading