-
Notifications
You must be signed in to change notification settings - Fork 3
🎨 Add post tx checks for the positive cases #166
base: fee-v3-load-existing-batches
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,7 @@ import ( | |||||||||
| "github.com/hibiken/asynq" | ||||||||||
| "github.com/sirupsen/logrus" | ||||||||||
| "github.com/vultisig/plugin/internal/types" | ||||||||||
| vtypes "github.com/vultisig/verifier/types" | ||||||||||
| "golang.org/x/sync/errgroup" | ||||||||||
| "golang.org/x/sync/semaphore" | ||||||||||
| ) | ||||||||||
|
|
@@ -38,7 +39,12 @@ func (fp *FeePlugin) HandlePostTx(ctx context.Context, task *asynq.Task) error { | |||||||||
| return fmt.Errorf("failed to acquire semaphore: %w", err) | ||||||||||
| } | ||||||||||
| defer sem.Release(1) | ||||||||||
| return fp.updateStatus(ctx, feeBatch, currentBlock) | ||||||||||
| if err := fp.updateStatus(ctx, feeBatch, currentBlock); err != nil { | ||||||||||
| fp.logger.WithField("batch_id", feeBatch.BatchID).Error("Failed to update fee batch status", err) | ||||||||||
| } else { | ||||||||||
| fp.logger.WithField("batch_id", feeBatch.BatchID).Info("Fee batch status update run successfully") | ||||||||||
| } | ||||||||||
| return nil | ||||||||||
|
Comment on lines
+42
to
+47
|
||||||||||
| }) | ||||||||||
| } | ||||||||||
| if err := eg.Wait(); err != nil { | ||||||||||
|
|
@@ -61,6 +67,8 @@ func (fp *FeePlugin) updateStatus(ctx context.Context, batch types.FeeBatch, cur | |||||||||
| fp.logger.WithFields(logrus.Fields{"batch_id": batch.BatchID}).Info("tx not found on chain, rebroadcasting") | ||||||||||
| return nil | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Tx successful | ||||||||||
| if receipt.Status == 1 { | ||||||||||
| if currentBlock > receipt.BlockNumber.Uint64()+fp.config.Jobs.Post.SuccessConfirmations { | ||||||||||
| fp.logger.WithFields(logrus.Fields{"batch_id": batch.BatchID}).Info("tx successful, setting to success") | ||||||||||
|
|
@@ -69,23 +77,32 @@ func (fp *FeePlugin) updateStatus(ctx context.Context, batch types.FeeBatch, cur | |||||||||
| if err != nil { | ||||||||||
| return err | ||||||||||
| } | ||||||||||
|
|
||||||||||
| var rollbackErr error | ||||||||||
| defer func() { | ||||||||||
| if rollbackErr != nil { | ||||||||||
| tx.Rollback(ctx) | ||||||||||
| } | ||||||||||
| }() | ||||||||||
|
|
||||||||||
| fp.verifierApi.UpdateFeeBatchTxHash(*batch.TxHash, batch.BatchID, *batch.TxHash) | ||||||||||
| hash := "" | ||||||||||
| if batch.TxHash != nil { | ||||||||||
| hash = *batch.TxHash | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if err = fp.db.SetFeeBatchStatus(ctx, tx, batch.BatchID, types.FeeBatchStateSuccess); err != nil { | ||||||||||
| resp, err := fp.verifierApi.UpdateFeeBatch(batch.PublicKey, batch.BatchID, hash, 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("failed to update verifier fee batch to success: %s", resp.Error.Message) | ||||||||||
| return fmt.Errorf("failed to update verifier fee batch to success: %s", resp.Error.Message) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if err = fp.db.SetFeeBatchStatus(ctx, tx, batch.BatchID, types.FeeBatchStateSuccess); err != nil { | ||||||||||
| rollbackErr = err | ||||||||||
| return fmt.Errorf("failed to set fee batch success: %w", err) | ||||||||||
| return fmt.Errorf("failed to update verifier fee batch to success: %w", err) | ||||||||||
|
||||||||||
| 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) |
garry-sharp marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Sep 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -150,8 +150,6 @@ func (fp *FeePlugin) initSign( | |||||||||||||||||
| feeBatch types.FeeBatch, | ||||||||||||||||||
| ) error { | ||||||||||||||||||
|
|
||||||||||||||||||
| fmt.Printf("Init sign: %+v\n", req) | ||||||||||||||||||
|
|
||||||||||||||||||
| sigs, err := fp.signer.Sign(ctx, req) | ||||||||||||||||||
| if err != nil { | ||||||||||||||||||
| fp.logger.WithError(err).Error("Keysign failed") | ||||||||||||||||||
|
|
@@ -193,11 +191,35 @@ func (fp *FeePlugin) initSign( | |||||||||||||||||
| return fmt.Errorf("failed to decode tx: %w", err) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| if err := fp.db.SetFeeBatchSent(ctx, txHash.Hash().Hex(), feeBatch.BatchID); err != nil { | ||||||||||||||||||
| tx, err := fp.db.Pool().Begin(ctx) | ||||||||||||||||||
| if err != nil { | ||||||||||||||||||
| return fmt.Errorf("failed to begin transaction: %w", err) | ||||||||||||||||||
| } | ||||||||||||||||||
| var rollbackErr error | ||||||||||||||||||
| defer func() { | ||||||||||||||||||
| if rollbackErr != nil { | ||||||||||||||||||
| tx.Rollback(ctx) | ||||||||||||||||||
| } | ||||||||||||||||||
| }() | ||||||||||||||||||
|
|
||||||||||||||||||
| if err := fp.db.SetFeeBatchSent(ctx, tx, txHash.Hash().Hex(), feeBatch.BatchID); err != nil { | ||||||||||||||||||
| rollbackErr = err | ||||||||||||||||||
| return fmt.Errorf("failed to set fee batch sent: %w", err) | ||||||||||||||||||
| } | ||||||||||||||||||
| resp, err := fp.verifierApi.UpdateFeeBatch(pluginPolicy.PublicKey, feeBatch.BatchID, txHash.Hash().Hex(), types.FeeBatchStateSent) | ||||||||||||||||||
| if err != nil { | ||||||||||||||||||
| rollbackErr = err | ||||||||||||||||||
| return fmt.Errorf("failed to update fee batch: %w", err) | ||||||||||||||||||
| } | ||||||||||||||||||
| if resp.Error.Message != "" { | ||||||||||||||||||
| rollbackErr = err | ||||||||||||||||||
|
Comment on lines
+214
to
+215
|
||||||||||||||||||
| if resp.Error.Message != "" { | |
| rollbackErr = err | |
| rollbackErr = fmt.Errorf("API error: %s", resp.Error.Message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.