🎨 Add post tx checks for the positive cases #166
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRenames and extends the verifier API method to accept a status and return an APIResponse wrapper, removes a RevertFeeCredit method, adjusts DB transaction usage for fee-batch state updates, updates transaction submission and post-tx handling to call the new API and handle API-level errors, and adds a failed-tx recovery routine. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant P as Plugin (transaction.go)
participant V as VerifierApi client
participant S as Verifier Service (HTTP)
P->>V: UpdateFeeBatch(pubKey, batchID, txHash, Sent)
V->>S: HTTP PUT /fee-batches/{id} { txHash, status: Sent }
S-->>V: 200 OK { data | error }
V-->>P: APIResponse[FeeBatchCreateResponseDto]
alt resp.Error.Message non-empty or HTTP error
P-->>P: rollback & return error
else success
P-->>P: commit tx and continue
end
sequenceDiagram
autonumber
participant H as PostTx Handler (post_tx.go)
participant N as Chain/Node
participant V as VerifierApi client
participant DB as Database (tx)
rect rgb(250,250,245)
note over H: For each batch (goroutine)
H->>N: Check tx receipt/status
alt Success && confirmations sufficient
H->>V: UpdateFeeBatch(..., Success)
V-->>H: APIResponse (check resp.Error.Message)
alt API ok
H->>DB: BEGIN
H->>DB: SetFeeBatchStatus(tx, batchID, Success)
H->>DB: COMMIT
else API error
H-->>H: log error, continue
end
else TX failed
H->>H: handleFailedTx(batchID)
H->>DB: BEGIN
H->>DB: SetFeeBatchStatus(tx, batchID, Failed)
H->>H: fetch policies & executeFeeLoading(policy)
H->>DB: COMMIT
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugin/fees/post_tx.go (2)
63-71: Guard on RPC error before using receipt; prefer errors.Is for NotFound.If TransactionReceipt returns a non-NotFound error, the code still reads receipt.Status, which can be undefined. Handle err first and use errors.Is for robustness.
Apply:
- receipt, err := fp.ethClient.TransactionReceipt(ctx, hash) - if err == ethereum.NotFound { + receipt, err := fp.ethClient.TransactionReceipt(ctx, hash) + if errors.Is(err, ethereum.NotFound) { // TODO rebroadcast logic fp.logger.WithFields(logrus.Fields{"batch_id": batch.BatchID}).Info("tx not found on chain, rebroadcasting") return nil - } + } + if err != nil { + return fmt.Errorf("failed to fetch tx receipt: %w", err) + }Add import:
import ( "context" "fmt" + "errors"
41-91: FixUpdateFeeBatchargument order in post_tx.go
In plugin/fees/post_tx.go (line 84), the first parameter passed toUpdateFeeBatchis*batch.TxHash(the tx hash) but must be the public key. Swap this to the correct publicKey value.
🧹 Nitpick comments (4)
internal/verifierapi/fees.go (1)
153-159: Naming nit: use publicKey for consistency.Minor style alignment with the rest of the file.
Apply:
-func (v *VerifierApi) UpdateFeeBatch(publickey string, batchId uuid.UUID, hash string, status types.FeeBatchState) (*APIResponse[FeeBatchCreateResponseDto], error) { +func (v *VerifierApi) UpdateFeeBatch(publicKey string, batchId uuid.UUID, hash string, status types.FeeBatchState) (*APIResponse[FeeBatchCreateResponseDto], error) { - response, err := v.putAuth("/fees/batch", FeeBatchUpdateRequestResponseDto{ - PublicKey: publickey, + response, err := v.putAuth("/fees/batch", FeeBatchUpdateRequestResponseDto{ + PublicKey: publicKey,plugin/fees/post_tx.go (3)
41-47: Swallowed per-batch errors: add visibility.Returning nil keeps the job “successful” even if all batches fail. Consider incrementing a metric or increasing log severity on error so mass failures are detectable.
73-83: Simplify tx lifecycle with always-defer rollback pattern.Defer tx.Rollback immediately; ignore rollback error after successful commit. This removes the need for rollbackErr bookkeeping and avoids leaks on early returns.
Apply:
- tx, err := fp.db.Pool().Begin(ctx) + tx, err := fp.db.Pool().Begin(ctx) if err != nil { return err } - var rollbackErr error - defer func() { - if rollbackErr != nil { - tx.Rollback(ctx) - } - }() + defer tx.Rollback(ctx) @@ - if err != nil { - rollbackErr = err - return fmt.Errorf("failed to update verifier fee batch to success: %w", err) - } + if err != nil { + return fmt.Errorf("failed to update verifier fee batch to success: %w", err) + } @@ - if err = fp.db.SetFeeBatchStatus(ctx, tx, batch.BatchID, types.FeeBatchStateSuccess); err != nil { - rollbackErr = err - return fmt.Errorf("failed to update verifier fee batch to success: %w", err) - } + if err = fp.db.SetFeeBatchStatus(ctx, tx, batch.BatchID, types.FeeBatchStateSuccess); err != nil { + return fmt.Errorf("failed to update verifier fee batch to success: %w", err) + } @@ - if err = tx.Commit(ctx); err != nil { - rollbackErr = err - return fmt.Errorf("failed to commit transaction: %w", err) - } + if err = tx.Commit(ctx); err != nil { + return fmt.Errorf("failed to commit transaction: %w", err) + }
109-110: Error handling on revert is ignored.Consider logging or propagating the error from RevertFeeCredit to aid incident response if the revert call fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
internal/verifierapi/fees.go(1 hunks)plugin/fees/post_tx.go(2 hunks)plugin/fees/transaction.go(1 hunks)storage/db.go(1 hunks)
🔇 Additional comments (2)
storage/db.go (1)
26-27: Interface change verified: SetFeeBatchTxHash removal complete and SetFeeBatchSent (non-transactional) vs SetFeeBatchStatus (transactional) split is intentional
No further action required.plugin/fees/post_tx.go (1)
70-76: Confirm confirmation threshold semantics.You’re using strict “>” instead of “>=”. If the policy is “N confirmations are enough,” prefer “>=” to avoid waiting one extra block.
Would you like me to align this to “>=” based on your confirmations policy?
plugin/fees/post_tx.go
Outdated
| resp, err := fp.verifierApi.UpdateFeeBatch(*batch.TxHash, batch.BatchID, *batch.TxHash, types.FeeBatchStateSuccess) | ||
| if err != nil { | ||
| rollbackErr = err | ||
| return fmt.Errorf("failed to update verifier fee batch to success: %w", err) | ||
| } | ||
| if resp.Error.Message != "" { | ||
| return fmt.Errorf("failed to update verifier fee batch to success: %s", resp.Error.Message) | ||
| } |
There was a problem hiding this comment.
Wrong argument and missing rollback: fix UpdateFeeBatch call and ensure tx rollback on API error.
- First arg to UpdateFeeBatch should be the public key, not the tx hash.
- When resp.Error.Message is non-empty, the DB tx is left unrolled back (rollbackErr not set).
Apply:
- resp, err := fp.verifierApi.UpdateFeeBatch(*batch.TxHash, batch.BatchID, *batch.TxHash, types.FeeBatchStateSuccess)
+ resp, err := fp.verifierApi.UpdateFeeBatch(batch.PublicKey, batch.BatchID, *batch.TxHash, types.FeeBatchStateSuccess)
if err != nil {
rollbackErr = err
return fmt.Errorf("failed to update verifier fee batch to success: %w", err)
}
- if resp.Error.Message != "" {
- return fmt.Errorf("failed to update verifier fee batch to success: %s", resp.Error.Message)
- }
+ if resp.Error.Message != "" {
+ rollbackErr = fmt.Errorf("verifier update error: %s", resp.Error.Message)
+ return fmt.Errorf("failed to update verifier fee batch to success: %s", resp.Error.Message)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resp, err := fp.verifierApi.UpdateFeeBatch(*batch.TxHash, batch.BatchID, *batch.TxHash, types.FeeBatchStateSuccess) | |
| if err != nil { | |
| rollbackErr = err | |
| return fmt.Errorf("failed to update verifier fee batch to success: %w", err) | |
| } | |
| if resp.Error.Message != "" { | |
| return fmt.Errorf("failed to update verifier fee batch to success: %s", resp.Error.Message) | |
| } | |
| resp, err := fp.verifierApi.UpdateFeeBatch(batch.PublicKey, batch.BatchID, *batch.TxHash, types.FeeBatchStateSuccess) | |
| if err != nil { | |
| rollbackErr = err | |
| return fmt.Errorf("failed to update verifier fee batch to success: %w", err) | |
| } | |
| if resp.Error.Message != "" { | |
| rollbackErr = fmt.Errorf("verifier update error: %s", resp.Error.Message) | |
| return fmt.Errorf("failed to update verifier fee batch to success: %s", resp.Error.Message) | |
| } |
🤖 Prompt for AI Agents
In plugin/fees/post_tx.go around lines 84 to 91, the UpdateFeeBatch call is
using batch.TxHash as the first argument but should use the batch's public key
(e.g. batch.PubKey or the field that holds the submitter's public key); change
the first parameter to that public key. Also, when resp.Error.Message is
non-empty set rollbackErr to an error (e.g. rollbackErr = fmt.Errorf(...))
before returning so the DB transaction will be rolled back, and return the
formatted error message. Ensure both the API call and the error-path
consistently set rollbackErr and return the error.
garry-sharp
force-pushed
the
fees-v3-post
branch
from
811a224 to
8545a17
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR unifies the fee batch update flow with improved error handling and structured API responses. The changes focus on making the transaction processing more reliable by adding proper transaction management and surfacing API errors.
- Refactored fee batch update methods to use a unified API approach with explicit status handling
- Added proper transaction commit/rollback handling for database operations
- Improved error handling by surfacing API errors instead of silent failures
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/postgres/fees.go | Updated GetFeeBatch query to handle multiple IDs and modified SetFeeBatchSent to use transaction |
| storage/db.go | Reorganized interface methods and removed obsolete SetFeeBatchTxHash method |
| plugin/fees/transaction.go | Added transaction management and unified API call for fee batch updates |
| plugin/fees/post_tx.go | Enhanced error handling with proper logging and added failed transaction handling |
| internal/verifierapi/fees.go | Unified fee batch update API with structured response handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| rollbackErr = err | ||
| return fmt.Errorf("failed to get plugin policy: %w", err) |
There was a problem hiding this comment.
The variable err is being assigned to rollbackErr but err is nil at this point since the previous operation succeeded. This should be rollbackErr = fmt.Errorf(\"no plugin policies found\") or similar.
| rollbackErr = err | |
| return fmt.Errorf("failed to get plugin policy: %w", err) | |
| rollbackErr = fmt.Errorf("no plugin policies found for public key %s", batch.PublicKey) | |
| return fmt.Errorf("no plugin policies found for public key %s", batch.PublicKey) |
| if resp.Error.Message != "" { | ||
| rollbackErr = err |
There was a problem hiding this comment.
The variable err is being assigned to rollbackErr but err is nil at this point since the API call succeeded. This should be rollbackErr = fmt.Errorf(\"API error: %s\", resp.Error.Message) or similar.
| if resp.Error.Message != "" { | |
| rollbackErr = err | |
| rollbackErr = fmt.Errorf("API error: %s", resp.Error.Message) |
storage/db.go
Outdated
| GetFeeBatchByStatus(ctx context.Context, status types.FeeBatchState) ([]types.FeeBatch, error) | ||
| CreateFeeBatch(ctx context.Context, tx pgx.Tx, batches ...types.FeeBatch) ([]types.FeeBatch, error) | ||
| SetFeeBatchStatus(ctx context.Context, tx pgx.Tx, batchId uuid.UUID, status types.FeeBatchState) error | ||
| SetFeeBatchSent(ctx context.Context, txHash string, batchId uuid.UUID) error |
There was a problem hiding this comment.
The SetFeeBatchSent method signature in the interface doesn't match the implementation which expects a tx pgx.Tx parameter. The interface should be updated to include the transaction parameter.
| SetFeeBatchSent(ctx context.Context, txHash string, batchId uuid.UUID) error | |
| SetFeeBatchSent(ctx context.Context, tx pgx.Tx, txHash string, batchId uuid.UUID) error |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
internal/verifierapi/fees.go (1)
143-149: Rename arg and fix message wording.
- Use consistent casing: publicKey.
- Error should say “update”, not “get”. This mirrors prior feedback.
-func (v *VerifierApi) UpdateFeeBatch(publickey string, batchId uuid.UUID, hash string, status types.FeeBatchState) (*APIResponse[FeeBatchCreateResponseDto], error) { +func (v *VerifierApi) UpdateFeeBatch(publicKey string, batchId uuid.UUID, hash string, status types.FeeBatchState) (*APIResponse[FeeBatchCreateResponseDto], error) { - response, err := v.putAuth("/fees/batch", FeeBatchUpdateRequestResponseDto{ - PublicKey: publickey, + response, err := v.putAuth("/fees/batch", FeeBatchUpdateRequestResponseDto{ + PublicKey: publicKey,- if err != nil { - return nil, fmt.Errorf("failed to get fee batch: %w", err) - } + if err != nil { + return nil, fmt.Errorf("failed to update fee batch: %w", err) + }plugin/fees/post_tx.go (3)
42-47: Fix log call: pass error via WithError.Current call passes err as a second arg to Error(), which is ignored. This mirrors prior feedback.
- fp.logger.WithField("batch_id", feeBatch.BatchID).Error("Failed to update fee batch status", err) + fp.logger.WithField("batch_id", feeBatch.BatchID).WithError(err).Error("Failed to update fee batch status")
97-100: Wrong error context.This error occurs when updating DB status, not the verifier. Prior feedback still applies.
- return fmt.Errorf("failed to update verifier fee batch to success: %w", err) + return fmt.Errorf("failed to set fee batch status to success: %w", err)
88-95: Unrolled-back tx on API-level error.If resp.Error.Message != "", you return without setting rollbackErr, leaking the tx. Set rollbackErr or adopt defer-rollback pattern.
- if resp.Error.Message != "" { - return fmt.Errorf("failed to update verifier fee batch to success: %s", resp.Error.Message) - } + if resp.Error.Message != "" { + rollbackErr = fmt.Errorf("verifier update error: %s", resp.Error.Message) + return fmt.Errorf("failed to update verifier fee batch to success: %s", resp.Error.Message) + }Optional simplification:
- var rollbackErr error - defer func(){ if rollbackErr != nil { tx.Rollback(ctx) } }() + defer func(){ _ = tx.Rollback(ctx) }()
🧹 Nitpick comments (6)
internal/verifierapi/fees.go (1)
155-160: Preserve API error details on non-200.Decode body first and include API error message when status != 200. Keeps parity with other methods and eases debugging.
- var feeBatchResponse APIResponse[FeeBatchCreateResponseDto] - if response.StatusCode != http.StatusOK { - return nil, fmt.Errorf("failed to update fee batch, status code: %d", response.StatusCode) - } - if err := json.NewDecoder(response.Body).Decode(&feeBatchResponse); err != nil { + var feeBatchResponse APIResponse[FeeBatchCreateResponseDto] + if err := json.NewDecoder(response.Body).Decode(&feeBatchResponse); err != nil { return nil, fmt.Errorf("failed to decode fee batch response: %w", err) } + if response.StatusCode != http.StatusOK { + if feeBatchResponse.Error.Message != "" { + return nil, fmt.Errorf("failed to update fee batch: %s", feeBatchResponse.Error.Message) + } + return nil, fmt.Errorf("failed to update fee batch, status code: %d", response.StatusCode) + }storage/postgres/fees.go (1)
83-86: Check rows affected for defensive safety.Optional: ensure exactly one row is updated and surface unexpected 0 updates.
- _, err := tx.Exec(ctx, query, types.FeeBatchStateSent, txHash, batchId) - return err + ct, err := tx.Exec(ctx, query, types.FeeBatchStateSent, txHash, batchId) + if err != nil { + return err + } + if ct.RowsAffected() == 0 { + return fmt.Errorf("no fee_batch updated for batch_id=%s", batchId) + } + return nilplugin/fees/post_tx.go (1)
132-133: Outdated comment.Comment says API creates a debit line, but code only updates DB and triggers fee loading. Update the comment.
- // This api call automatically creates a new debit line for the failed tx, which will get picked up in a new batch. + // Mark batch as failed and immediately trigger fee loading to pick up a new debit line in the next batch.plugin/fees/transaction.go (3)
199-203: Harden rollback defer; log rollback failure.Capture and log rollback errors to avoid silent failures in cleanup.
Apply:
- defer func() { - if rollbackErr != nil { - tx.Rollback(ctx) - } - }() + defer func() { + if rollbackErr != nil { + if rbErr := tx.Rollback(ctx); rbErr != nil { + fp.logger.WithError(rbErr).Warn("rollback failed") + } + } + }()
205-218: DB-first-in-tx is safer, but commit-failure still risks API/DB divergence.Your gating (write in DB tx → call API → commit) fixes “DB shows Sent when API failed.” However, if API succeeds and commit later fails, API=Sent while DB≠Sent. Consider an outbox or a “Sending/SentPending” state with a reconciler to finalize API after commit (or vice versa), so either side can be repaired deterministically.
Do we already have a reconciler/job that heals API/DB state on commit errors?
86-90: Fix log formatting: use Errorf.The
%dwon’t be formatted withError(...). UseErrorf(...).Apply:
- }).Error("Expected 1 plugin policy, got %d", len(policies)) + }).Errorf("Expected 1 plugin policy, got %d", len(policies))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
internal/verifierapi/fees.go(1 hunks)plugin/fees/post_tx.go(5 hunks)plugin/fees/transaction.go(2 hunks)storage/db.go(1 hunks)storage/postgres/fees.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- storage/db.go
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-24T19:34:47.441Z
Learnt from: webpiratt
PR: vultisig/plugin#141
File: plugin/payroll/transaction.go:208-267
Timestamp: 2025-07-24T19:34:47.441Z
Learning: In the vultisig/plugin codebase, duplicate transaction prevention for the payroll plugin has been moved to the verifier side, so the local `IsAlreadyProposed` check in plugin/payroll/transaction.go is no longer needed. The validation flow has changed and is now handled by the verifier rather than within the plugin itself.
Applied to files:
plugin/fees/post_tx.goplugin/fees/transaction.go
📚 Learning: 2025-08-20T13:51:05.212Z
Learnt from: neavra
PR: vultisig/plugin#157
File: api/plugin.go:236-236
Timestamp: 2025-08-20T13:51:05.212Z
Learning: In the vultisig/plugin codebase migration to vultisig-go, SystemMigrations() and vultisig_validator functionality were not moved from the verifier package to vultisig-go. This means some imports of github.com/vultisig/verifier/common are intentionally retained for accessing these specific functions, while vault-related types and common functionality were migrated to vultisig-go.
Applied to files:
plugin/fees/post_tx.go
📚 Learning: 2025-05-07T08:23:45.882Z
Learnt from: RaghavSood
PR: vultisig/plugin#36
File: api/server.go:21-33
Timestamp: 2025-05-07T08:23:45.882Z
Learning: The import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides the plugin interface, and should not be changed to `github.com/vultisig/plugin/plugin` as these are distinct packages with different purposes.
Applied to files:
plugin/fees/post_tx.go
📚 Learning: 2025-07-31T15:43:42.631Z
Learnt from: garry-sharp
PR: vultisig/plugin#146
File: plugin/fees/transaction.go:158-160
Timestamp: 2025-07-31T15:43:42.631Z
Learning: In the vultisig/plugin fees plugin, the public ProposeTransactions method intentionally returns "not implemented" because the fees plugin v2 uses a run-based architecture instead of the standard plugin template pattern. The actual transaction proposal logic is implemented in the private proposeTransactions method that accepts a types.FeeRun parameter, allowing fees to be batched and tracked through fee runs rather than individual policy-based transactions.
Applied to files:
plugin/fees/transaction.go
🔇 Additional comments (6)
internal/verifierapi/fees.go (1)
162-162: Return type usage LGTM.Returning the wrapper aligns with callers that inspect resp.Error.
plugin/fees/post_tx.go (1)
139-149: Ignore misspelling suggestion for PluginVultisigFees_feee
The constantvtypes.PluginVultisigFees_feeeis defined in the external dependency with three ‘e’s; no change required.Likely an incorrect or invalid review comment.
plugin/fees/transaction.go (4)
219-222: On commit error, attempt rollback but also schedule reconciliation.Even with the defer, after a commit error the API may already be “Sent”. Ensure a compensating task reconciles API/DB status if commit fails (e.g., mark API “needs_sync” or enqueue a resync).
I can propose a small “fees:state-reconcile” job if desired.
233-233: LGTM: capture broadcast tx for accurate logging.
240-246: LGTM: use broadcast hash in success log.
78-85: Verify enum namePluginVultisigFees_feee.Triple “e” looks accidental. If the enum is
PluginVultisigFees_fee(or similar), this won’t compile.Would you confirm the intended constant?
| // This function sets a batch id to be failed and requests for a new debit line to be created. The failed tx then gets picked up in a new batch. | ||
| func (fp *FeePlugin) handleFailedTx(ctx context.Context, batch types.FeeBatch) error { | ||
| fp.logger.WithFields(logrus.Fields{"batch_id": batch.BatchID}).Info("tx failed, setting to failed") | ||
|
|
||
| tx, err := fp.db.Pool().Begin(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| var rollbackErr error | ||
| defer func() { | ||
| if rollbackErr != nil { | ||
| tx.Rollback(ctx) | ||
| } | ||
| }() | ||
|
|
||
| // This api call automatically creates a new debit line for the failed tx, which will get picked up in a new batch. | ||
| err = fp.db.SetFeeBatchStatus(ctx, tx, batch.BatchID, types.FeeBatchStateFailed) | ||
| if err != nil { | ||
| rollbackErr = err | ||
| return fmt.Errorf("failed to set fee batch status to failed: %w", err) | ||
| } | ||
|
|
||
| feePolicies, err := fp.db.GetPluginPolicies(ctx, batch.PublicKey, vtypes.PluginVultisigFees_feee, true) | ||
| if err != nil { | ||
| rollbackErr = err | ||
| return fmt.Errorf("failed to get plugin policy: %w", err) | ||
| } | ||
|
|
||
| if len(feePolicies) < 1 { | ||
| rollbackErr = err | ||
| return fmt.Errorf("failed to get plugin policy: %w", err) | ||
| } | ||
|
|
||
| feePolicy := feePolicies[0] | ||
|
|
||
| // Immediately load a new fee batch | ||
| return fp.executeFeeLoading(ctx, feePolicy) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Transaction leak: missing commit before returning.
handleFailedTx starts a tx but never commits; returning executeFeeLoading leaves the tx open. Commit before triggering loading; keep rollback as safety.
// Immediately load a new fee batch
- return fp.executeFeeLoading(ctx, feePolicy)
+ if err = tx.Commit(ctx); err != nil {
+ return fmt.Errorf("failed to commit transaction: %w", err)
+ }
+ return fp.executeFeeLoading(ctx, feePolicy)Also consider unconditional deferred rollback to simplify:
- var rollbackErr error
- defer func(){ if rollbackErr != nil { tx.Rollback(ctx) } }()
+ defer func(){ _ = tx.Rollback(ctx) }()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugin/fees/post_tx.go around lines 117-154, the function begins a DB
transaction but never commits it before returning, leaking the open tx; change
the flow to use an unconditional deferred rollback (defer tx.Rollback(ctx)) and
after successfully setting the batch status and loading feePolicy, call
tx.Commit(ctx) and check its error before calling fp.executeFeeLoading; only
call executeFeeLoading after commit succeeds and return any commit error (do not
return while tx remains open).
| if resp.Error.Message != "" { | ||
| rollbackErr = err | ||
| return fmt.Errorf("failed to update fee batch: %s", resp.Error.Message) | ||
| } |
There was a problem hiding this comment.
Critical: rollback never triggers on API-level error (leaks transaction/connection).
rollbackErr = err assigns nil (transport succeeded) when resp.Error.Message is set, so the deferred rollback won’t run. The tx stays open and the connection is pinned.
Apply:
- if resp.Error.Message != "" {
- rollbackErr = err
- return fmt.Errorf("failed to update fee batch: %s", resp.Error.Message)
- }
+ if resp.Error.Message != "" {
+ rollbackErr = fmt.Errorf("verifier api error: %s", resp.Error.Message)
+ return rollbackErr
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if resp.Error.Message != "" { | |
| rollbackErr = err | |
| return fmt.Errorf("failed to update fee batch: %s", resp.Error.Message) | |
| } | |
| if resp.Error.Message != "" { | |
| rollbackErr = fmt.Errorf("verifier api error: %s", resp.Error.Message) | |
| return rollbackErr | |
| } |
🤖 Prompt for AI Agents
In plugin/fees/transaction.go around lines 214 to 217, the deferred rollback
never runs because rollbackErr is set to err (which is nil) when
resp.Error.Message is present; change the code to set rollbackErr to a non-nil
error built from resp.Error.Message (e.g., rollbackErr = fmt.Errorf("API error
updating fee batch: %s", resp.Error.Message)) before returning so the deferred
rollback runs and the transaction/connection is released.
storage/postgres/fees.go
Outdated
| query := `select * from fee_batch where id = ANY($1)` | ||
| rows, err := p.pool.Query(ctx, query, batchIDs) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential field/param mismatch and array binding.
- Name says batchIDs but query filters on id. If intent is batch_id, switch the column.
- For ANY($1), pass a PG array to avoid codec issues.
- query := `select * from fee_batch where id = ANY($1)`
- rows, err := p.pool.Query(ctx, query, batchIDs)
+ query := `select * from fee_batch where batch_id = ANY($1::uuid[])`
+ rows, err := p.pool.Query(ctx, query, pgx.Array(batchIDs))To confirm callers expect batch_id, run:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 '\bGetFeeBatch\s*\('Length of output: 1113
🏁 Script executed:
#!/usr/bin/env bash
# Search for fee_batch table schema definitions in SQL migration files
echo "Searching for `create table fee_batch` definitions..."
rg -nR -n "(?i)create table.*fee_batch" -g "*.sql"
echo
echo "Searching for any `fee_batch` references in SQL files..."
rg -nR -n "fee_batch" -g "*.sql"Length of output: 1309
🏁 Script executed:
#!/usr/bin/env bash
# Find type definition of FeeBatch
rg -nP 'type\s+FeeBatch\b' -g '*.go' || echo 'No direct match for type FeeBatch'
# Find struct body for FeeBatch
rg -nP 'struct\s+FeeBatch' -C2 -g '*.go' || echo 'No match for struct FeeBatch'
# Search for BatchID field declarations
rg -nP 'BatchID' -C2 -g '*.go' || echo 'No match for BatchID'
# Search for any batch_id column references in Go code
rg -n 'batch_id' -g '*.go' || echo 'No in-code references to batch_id'Length of output: 11467
Filter by batch_id and bind batchIDs as uuid array
In GetFeeBatch (storage/postgres/fees.go:46–47), update:
- query := `select * from fee_batch where id = ANY($1)`
- rows, err := p.pool.Query(ctx, query, batchIDs)
+ query := `select * from fee_batch where batch_id = ANY($1::uuid[])`
+ rows, err := p.pool.Query(ctx, query, pgx.Array(batchIDs))This aligns the parameter with the batch_id column (db:"batch_id") and ensures proper UUID array binding.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| query := `select * from fee_batch where id = ANY($1)` | |
| rows, err := p.pool.Query(ctx, query, batchIDs) | |
| query := `select * from fee_batch where batch_id = ANY($1::uuid[])` | |
| rows, err := p.pool.Query(ctx, query, pgx.Array(batchIDs)) |
🤖 Prompt for AI Agents
In storage/postgres/fees.go around lines 46 to 47, the query is filtering on id
and not casting the parameter as a UUID array; change the SQL to filter by
batch_id and bind the parameter as a uuid[] (for example: `select * from
fee_batch where batch_id = ANY($1::uuid[])`) and ensure the call passes the
batchIDs slice as the single parameter so the driver treats it as a UUID array.
8545a17 to
99c7265
Compare
garry-sharp
force-pushed
the
fee-v3-load-existing-batches
branch
from
56b5b74 to
028ef1e
Compare
garry-sharp
force-pushed
the
fees-v3-post
branch
4 times, most recently
from
2a1c212 to
5a7dee8
Compare
garry-sharp
force-pushed
the
fee-v3-load-existing-batches
branch
from
028ef1e to
1cec5d5
Compare
garry-sharp
force-pushed
the
fees-v3-post
branch
from
5a7dee8 to
a670cea
Compare
garry-sharp
force-pushed
the
fees-v3-post
branch
from
a670cea to
a444c13
Compare
1 participant
Summary by CodeRabbit