✨ Resend stuck txs if nonce is the same as pending#148
Conversation
|
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 WalkthroughThis change introduces transactional handling and rebroadcast logic for fee-related Ethereum transactions. It adds a new database table to store fee run transactions, updates configuration to support a dry-run mode, and refactors how Ethereum addresses are derived and transactions are signed. Multiple storage and plugin methods are updated to support explicit transaction context, ensuring atomicity and consistency. Changes
Sequence Diagram(s)Fee Transaction Proposal, Signing, and Broadcast (with Transactional DB and Dry-Run Support)sequenceDiagram
participant User
participant FeePlugin
participant DB
participant EthereumClient
User->>FeePlugin: Propose Transaction
FeePlugin->>FeePlugin: Derive Ethereum address from policy
FeePlugin->>FeePlugin: Create unsigned transaction
FeePlugin->>DB: Begin Transaction
FeePlugin->>DB: Record FeeRun, Fee, FeeRunTx
FeePlugin->>FeePlugin: Sign transaction
alt DryRun is false
FeePlugin->>EthereumClient: Broadcast signed transaction
EthereumClient-->>FeePlugin: Return tx hash
FeePlugin->>DB: Commit Transaction
else DryRun is true
FeePlugin->>FeePlugin: Log dry-run (no broadcast)
FeePlugin->>DB: Commit Transaction
end
Fee Transaction Rebroadcast LogicsequenceDiagram
participant FeePlugin
participant DB
participant EthereumClient
FeePlugin->>EthereumClient: Query tx receipt
alt Receipt not found
FeePlugin->>DB: Get last FeeRunTx
FeePlugin->>FeePlugin: Decode tx, get sender address
FeePlugin->>EthereumClient: Get pending nonce for sender
alt Nonce matches tx nonce
FeePlugin->>DB: Begin Transaction
FeePlugin->>DB: Store FeeRunTx again
FeePlugin->>EthereumClient: Rebroadcast tx
FeePlugin->>DB: Commit Transaction
else Nonce does not match
FeePlugin->>FeePlugin: Log / TODO for other cases
end
else Receipt found
FeePlugin->>DB: Mark FeeRun as success (in transaction)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
To be merged into main AFTER #146 is merged @coderabbitai review |
|
@garry-sharp I'll review the changes in this PR. I've noted that this should be merged after PR #146 is completed. 🧠 Learnings used✅ 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: 5
🔭 Outside diff range comments (3)
storage/postgres/migrations/plugin/20250630152230_fee_runs.sql (1)
66-85: Missing DROP statement for fee_run_tx table in down migration.The down migration should include dropping the
fee_run_txtable to properly reverse the up migration.-- Drop tables (in reverse order due to foreign key constraints) +DROP TABLE IF EXISTS fee_run_tx; DROP TABLE IF EXISTS fee; DROP TABLE IF EXISTS fee_run;plugin/fees/helper.go (1)
24-41: Remove duplicate function implementation.The
getHashandappendSignaturefunctions have identical implementations. This creates maintenance overhead and potential for inconsistencies.Remove the duplicate
getHashfunction and update all references to useappendSignature:-func getHash(inTx evm.UnsignedTx, r, s, v []byte, chainID *big.Int) (*etypes.Transaction, error) { - var sig []byte - sig = append(sig, r...) - sig = append(sig, s...) - sig = append(sig, v...) - - inTxDecoded, err := reth.DecodeUnsignedPayload(inTx) - if err != nil { - return nil, fmt.Errorf("reth.DecodeUnsignedPayload: %w", err) - } - - outTx, err := etypes.NewTx(inTxDecoded).WithSignature(etypes.LatestSignerForChainID(chainID), sig) - if err != nil { - return nil, fmt.Errorf("types.NewTx.WithSignature: %w", err) - } - - return outTx, nil -}Also applies to: 134-151
plugin/fees/transaction.go (1)
182-182: Update to use appendSignature instead of getHash.Since
getHashandappendSignatureare duplicates, and the recommendation is to removegetHash, this call should be updated.-txHash, err := getHash(txBytes, r, s, v, fp.config.ChainId) +txHash, err := appendSignature(txBytes, r, s, v, fp.config.ChainId)
🧹 Nitpick comments (4)
etc/vultisig/fee.yml (1)
17-17: Add newline at end of file.The
dry_runconfiguration is a good addition for testing. However, please add a newline character at the end of the file to follow YAML best practices.-dry_run: false \ No newline at end of file +dry_run: false +plugin/fees/post_tx.go (2)
72-136: Well-implemented transaction rebroadcast logic.The implementation correctly handles stuck transactions by:
- Fetching the last transaction from the database
- Checking if the nonce matches the pending nonce
- Rebroadcasting within a database transaction for atomicity
The error handling and transaction management are properly implemented.
134-134: Address the TODO for nonce handling.The TODO indicates incomplete implementation for handling earlier or later nonces. This could lead to missed transactions or incorrect state.
Would you like me to implement the logic for handling different nonce scenarios or create an issue to track this?
plugin/fees/helper.go (1)
81-125: Consider adding type constraints to generic function.While the generic
parseErc20Txfunction is flexible, it could benefit from interface constraints to ensure type safety at compile time.Consider defining an interface for supported transaction types:
type TransactionData interface { Data() []byte To() *ecommon.Address }Then constrain the generic function:
-func parseErc20Tx[T any](transaction T) (*erc20Data, error) { +func parseErc20Tx[T TransactionData](transaction T) (*erc20Data, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
etc/vultisig/fee.yml(1 hunks)internal/types/fees.go(1 hunks)plugin/fees/config.go(6 hunks)plugin/fees/fees.go(3 hunks)plugin/fees/helper.go(4 hunks)plugin/fees/post_tx.go(4 hunks)plugin/fees/transaction.go(4 hunks)storage/db.go(2 hunks)storage/postgres/fees.go(4 hunks)storage/postgres/migrations/plugin/20250630152230_fee_runs.sql(1 hunks)storage/postgres/schema/schema.sql(3 hunks)
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
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.
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.
Learnt from: webpiratt
PR: vultisig/plugin#105
File: plugin/dca/dca.go:734-736
Timestamp: 2025-07-01T17:35:35.277Z
Learning: The DCA plugin in the vultisig/plugin codebase is currently not fully implemented. Methods like getCompletedSwapTransactionsCount() return placeholder values (e.g., 0) with TODO comments, which is expected behavior during the development phase. The plugin implementation is intentionally incomplete while other parts of the system are being refactored.
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/policy.go:46-47
Timestamp: 2025-07-04T10:47:47.927Z
Learning: For the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are intentionally configured to accept only USDC transfers ("ethereum.usdc.transfer"), not general ERC20 transfers ("ethereum.erc20.transfer"), as fees are only collected in USDC.
Learnt from: webpiratt
PR: vultisig/plugin#96
File: plugin/payroll/transaction.go:43-44
Timestamp: 2025-06-18T18:22:06.358Z
Learning: In the vultisig/plugin codebase, the hardcoded ethereumEvmChainID = big.NewInt(1) in plugin/payroll/transaction.go is intentional for the current implementation phase. The team is implementing ETH first, with plans to add other EVM chains later. The functions/methods are already designed to work with all EVM chains.
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/constraints.go:3-3
Timestamp: 2025-07-04T10:50:42.832Z
Learning: For the vultisig/plugin project, use "fee" (singular) throughout the codebase wherever possible, not "fees" (plural). This applies to all naming conventions including plugin types, variable names, function names, comments, and documentation.
📚 Learning: in the vultisig/plugin fees plugin, the public proposetransactions method intentionally returns "not...
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:
internal/types/fees.gostorage/postgres/migrations/plugin/20250630152230_fee_runs.sqletc/vultisig/fee.ymlplugin/fees/fees.goplugin/fees/post_tx.gostorage/db.goplugin/fees/transaction.goplugin/fees/config.goplugin/fees/helper.gostorage/postgres/fees.go
📚 Learning: the constant `vtypes.pluginvultisigfees_feee` in the vultisig/plugin project is correctly spelled wi...
Learnt from: webpiratt
PR: vultisig/plugin#126
File: plugin/fees/policy.go:26-26
Timestamp: 2025-07-10T20:41:44.025Z
Learning: The constant `vtypes.PluginVultisigFees_feee` in the vultisig/plugin project is correctly spelled with "feee" (including the extra 'e'). This is the actual constant name defined in the external dependency github.com/vultisig/verifier/types and should not be changed to "fee" singular, as this is how it's defined in the external package.
Applied to files:
internal/types/fees.goetc/vultisig/fee.ymlplugin/fees/fees.goplugin/fees/transaction.goplugin/fees/config.goplugin/fees/helper.go
📚 Learning: for the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are in...
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/policy.go:46-47
Timestamp: 2025-07-04T10:47:47.927Z
Learning: For the fee plugin in plugin/fees/policy.go, the resource validation and recipe specification are intentionally configured to accept only USDC transfers ("ethereum.usdc.transfer"), not general ERC20 transfers ("ethereum.erc20.transfer"), as fees are only collected in USDC.
Applied to files:
internal/types/fees.goplugin/fees/fees.goplugin/fees/post_tx.gostorage/db.goplugin/fees/transaction.goplugin/fees/config.goplugin/fees/helper.gostorage/postgres/fees.go
📚 Learning: in the `tx_indexer` table (storage/postgres/migrations/plugin/*_tx_indexer.sql), the `policy_id` col...
Learnt from: webpiratt
PR: vultisig/plugin#89
File: storage/postgres/migrations/plugin/20250603181247_tx_indexer.sql:13-13
Timestamp: 2025-06-11T18:42:54.241Z
Learning: In the `tx_indexer` table (storage/postgres/migrations/plugin/*_tx_indexer.sql), the `policy_id` column is intentionally **not** defined with a foreign-key constraint to `plugin_policies(id)`.
Applied to files:
storage/postgres/migrations/plugin/20250630152230_fee_runs.sqlstorage/postgres/schema/schema.sql
📚 Learning: for the vultisig/plugin project, use "fee" (singular) throughout the codebase wherever possible, not...
Learnt from: garry-sharp
PR: vultisig/plugin#117
File: plugin/fees/constraints.go:3-3
Timestamp: 2025-07-04T10:50:42.832Z
Learning: For the vultisig/plugin project, use "fee" (singular) throughout the codebase wherever possible, not "fees" (plural). This applies to all naming conventions including plugin types, variable names, function names, comments, and documentation.
Applied to files:
etc/vultisig/fee.ymlplugin/fees/fees.goplugin/fees/transaction.goplugin/fees/config.goplugin/fees/helper.go
📚 Learning: in the vultisig/plugin codebase, duplicate transaction prevention for the payroll plugin has been mo...
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:
etc/vultisig/fee.ymlplugin/fees/fees.goplugin/fees/post_tx.gostorage/db.goplugin/fees/transaction.goplugin/fees/config.goplugin/fees/helper.gostorage/postgres/fees.go
📚 Learning: in the payroll plugin (plugin/payroll/transaction.go), signrequest.transaction is stored with the "0...
Learnt from: webpiratt
PR: vultisig/plugin#96
File: plugin/payroll/transaction.go:0-0
Timestamp: 2025-06-18T18:23:20.077Z
Learning: In the payroll plugin (plugin/payroll/transaction.go), signRequest.Transaction is stored with the "0x" prefix, making it compatible with gcommon.FromHex which requires 0x-prefixed hex strings.
Applied to files:
plugin/fees/fees.goplugin/fees/transaction.goplugin/fees/helper.gostorage/postgres/fees.go
📚 Learning: the embed.fs directive `//go:embed migrations/plugin/*.sql` in storage/postgres/db_plugin.go correct...
Learnt from: RaghavSood
PR: vultisig/plugin#75
File: storage/postgres/db_plugin.go:25-40
Timestamp: 2025-05-30T02:44:31.711Z
Learning: The embed.FS directive `//go:embed migrations/plugin/*.sql` in storage/postgres/db_plugin.go correctly embeds plugin migration files that exist in the storage/postgres/migrations/plugin/ directory. The path "migrations/plugin" passed to goose.Up() references the embedded filesystem structure, not the physical file system path.
Applied to files:
storage/db.gostorage/postgres/fees.go
📚 Learning: in the vultisig/plugin codebase, the hardcoded ethereumevmchainid = big.newint(1) in plugin/payroll/...
Learnt from: webpiratt
PR: vultisig/plugin#96
File: plugin/payroll/transaction.go:43-44
Timestamp: 2025-06-18T18:22:06.358Z
Learning: In the vultisig/plugin codebase, the hardcoded ethereumEvmChainID = big.NewInt(1) in plugin/payroll/transaction.go is intentional for the current implementation phase. The team is implementing ETH first, with plans to add other EVM chains later. The functions/methods are already designed to work with all EVM chains.
Applied to files:
plugin/fees/transaction.goplugin/fees/config.goplugin/fees/helper.go
📚 Learning: in plugin/payroll/transaction.go, the gettokenid function is designed to return evm.zeroaddress.hex(...
Learnt from: webpiratt
PR: vultisig/plugin#125
File: plugin/payroll/transaction.go:186-197
Timestamp: 2025-07-09T22:22:36.651Z
Learning: In plugin/payroll/transaction.go, the getTokenID function is designed to return evm.ZeroAddress.Hex() when no "token" parameter constraint is found, rather than returning an error. This is expected behavior according to the team's design intent, even though the token parameter is marked as required in the recipe specification.
Applied to files:
plugin/fees/transaction.goplugin/fees/config.goplugin/fees/helper.go
📚 Learning: the import path `github.com/vultisig/verifier/plugin` refers to an external dependency that provides...
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/transaction.goplugin/fees/helper.go
📚 Learning: in the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers d...
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: Dockerfile.Payroll.server:14-19
Timestamp: 2025-07-02T04:55:36.331Z
Learning: In the vultisig/plugin repository, the team maintains both the main repository and the go-wrappers dependency repository, so they are comfortable downloading from the master branch rather than pinning to specific commits.
Applied to files:
plugin/fees/transaction.go
📚 Learning: vaultserviceconfig is defined as a field of type vault_config.config from the external package "gith...
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config from the external package "github.com/vultisig/verifier/vault_config" in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). The vault_config.Config struct contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
Applied to files:
plugin/fees/transaction.goplugin/fees/config.go
📚 Learning: in plugin/payroll/policy.go, the "token" case in the checkrule method intentionally does not call va...
Learnt from: webpiratt
PR: vultisig/plugin#125
File: plugin/payroll/policy.go:160-160
Timestamp: 2025-07-09T22:23:17.348Z
Learning: In plugin/payroll/policy.go, the "token" case in the checkRule method intentionally does not call validateToken() as this is a temporary implementation that will be replaced with generic schema validation. The missing validation call is expected behavior during this transition period.
Applied to files:
plugin/fees/transaction.go
📚 Learning: in the vultisig/plugin codebase, magic constants (constraint_type_magic_constant) use getfixedvalue(...
Learnt from: garry-sharp
PR: vultisig/plugin#143
File: plugin/fees/transaction.go:72-80
Timestamp: 2025-07-28T23:15:39.445Z
Learning: In the vultisig/plugin codebase, magic constants (CONSTRAINT_TYPE_MAGIC_CONSTANT) use GetFixedValue() to retrieve their values, not a separate GetMagicConstantValue() method. The constraint type validation ensures it's the correct type, but value retrieval uses the unified GetFixedValue() method across all constraint types.
Applied to files:
plugin/fees/transaction.go
📚 Learning: vaultserviceconfig is defined as a field of type vault_config.config in worker configuration structs...
Learnt from: johnnyluo
PR: vultisig/plugin#108
File: cmd/payroll/worker/main.go:84-84
Timestamp: 2025-07-02T04:58:30.139Z
Learning: VaultServiceConfig is defined as a field of type vault_config.Config in worker configuration structs across the vultisig/plugin codebase (cmd/payroll/worker/config.go, cmd/fees/worker/config.go, cmd/dca/worker/config.go). It contains an EncryptionSecret field that can be accessed via cfg.VaultServiceConfig.EncryptionSecret.
Applied to files:
plugin/fees/config.go
📚 Learning: the erc20abi constant is defined in plugin/payroll/constants.go within the payroll package, making i...
Learnt from: webpiratt
PR: vultisig/plugin#96
File: plugin/payroll/transaction.go:510-514
Timestamp: 2025-06-18T18:20:59.510Z
Learning: The erc20ABI constant is defined in plugin/payroll/constants.go within the payroll package, making it accessible to other files in the same package like transaction.go.
Applied to files:
plugin/fees/helper.go
🧬 Code Graph Analysis (3)
plugin/fees/fees.go (1)
internal/types/fees.go (1)
FeeRunStateDraft(14-14)
storage/db.go (2)
internal/types/fees.go (4)
FeeRunState(11-11)FeeRun(29-39)Fee(21-26)FeeRunTx(41-48)internal/verifierapi/fees.go (1)
FeeDto(12-21)
storage/postgres/fees.go (3)
storage/postgres/db.go (1)
PostgresBackend(15-17)internal/types/fees.go (5)
FeeRunState(11-11)FeeRun(29-39)FeeRunStateSent(15-15)FeeRunStateSuccess(16-16)FeeRunTx(41-48)internal/verifierapi/fees.go (1)
FeeDto(12-21)
🪛 YAMLlint (1.37.1)
etc/vultisig/fee.yml
[error] 17-17: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (21)
internal/types/fees.go (1)
41-48: LGTM! Well-structured type definition.The
FeeRunTxstruct is properly defined with appropriate field types and database tags that align with the schema defined in the migration files.plugin/fees/fees.go (1)
206-222: Good transaction handling implementation.The transaction management with proper error handling and deferred commit/rollback ensures atomicity when creating fee runs and adding fees.
storage/postgres/schema/schema.sql (1)
79-87: LGTM! Schema properly defines fee_run_tx table.The table definition with appropriate constraints supports the transaction tracking functionality and maintains referential integrity with cascade delete.
Also applies to: 145-147, 189-191
plugin/fees/post_tx.go (2)
56-56: Good addition of error logging.Adding error logging here improves observability when fee run status checks fail.
147-168: Excellent transactional consistency improvement.Wrapping both the database update and verifier API call in a single transaction ensures atomicity and prevents state mismatches. The error handling with proper rollback/commit semantics is well implemented.
plugin/fees/config.go (3)
14-42: Well-designed configuration refactoring.The introduction of
FeeConfigFileWrapperto handle unmarshaling and type conversion is a clean solution. ConvertingChainIdto*big.Intprovides better type safety and aligns with Ethereum standards.
36-36: Good addition of dry-run support.The
DryRunfield with proper default value and configuration option enables safe testing of transaction flows without broadcasting to the network.Also applies to: 60-60, 109-114
169-171: Improved validation for ChainId.The enhanced validation ensures ChainId is neither nil nor zero, preventing potential runtime errors.
storage/db.go (2)
26-28: Excellent enforcement of transactional consistency.Requiring explicit
pgx.Txparameters for fee run and fee operations ensures atomicity across related database operations. This prevents partial updates and maintains data integrity.Also applies to: 32-32
34-35: Well-designed transaction storage methods.The new
CreateFeeRunTxandGetFeeRunTxsmethods properly support the transaction rebroadcast feature by storing and retrieving transaction details with appropriate metadata including chain ID.plugin/fees/helper.go (2)
61-79: Well-implemented transaction decoding.The
decodeUnsignedTxfunction properly validates the transaction type and handles RLP decoding with appropriate error messages.
153-174: Excellent abstraction for address derivation.The new methods properly encapsulate the logic for deriving Ethereum addresses from fee policies, with good error handling and clear separation of concerns.
plugin/fees/transaction.go (3)
37-40: Clean abstraction for address retrieval.Using the helper method
getEthAddressFromFeePolicyimproves code organization and reusability.
215-246: Excellent transaction management and dry-run support.The implementation properly manages database transactions with appropriate rollback/commit logic and integrates dry-run functionality for safe testing. The error handling ensures atomicity between database state and blockchain transactions.
187-214: Good refactoring of transaction parsing logic.Using the new helper functions
decodeUnsignedTx,parseErc20Tx, andappendSignatureimproves code organization and reusability.storage/postgres/fees.go (6)
7-9: LGTM! New imports align with added functionality.The new imports for
math/bigandhexutilare appropriately added to support the big integer handling and hex encoding functionality in the newCreateFeeRunTxmethod.
62-63: LGTM! Method correctly uses provided transaction parameter.The method signature has been updated to accept a
pgx.Txparameter and the implementation correctly usesdbTx.Exec()instead of the connection pool directly, enabling proper transactional consistency.
70-71: LGTM! Method correctly uses provided transaction parameter.The method signature has been updated to accept a
pgx.Txparameter and the implementation correctly usesdbTx.Exec()instead of the connection pool directly, maintaining transactional consistency.
175-176: LGTM! Method correctly uses provided transaction parameter.The method signature has been updated to accept a
pgx.Txparameter and the implementation correctly usesdbTx.Exec()instead of the connection pool directly, ensuring transactional consistency.
203-214: LGTM! Well-implemented method for storing fee run transactions.The
CreateFeeRunTxmethod correctly:
- Uses the provided transaction parameter for database operations
- Properly encodes the transaction bytes to hex using
hexutil.Encode()- Safely handles potential nil
chainIDby converting to uint64- Provides appropriate error handling and formatting
The implementation aligns with the
FeeRunTxstruct definition and supports the transaction tracking functionality mentioned in the PR objectives.
217-234: LGTM! Well-implemented method for retrieving fee run transactions.The
GetFeeRunTxsmethod correctly:
- Uses appropriate ordering (most recent first) as documented in the comment
- Properly handles database connection and row scanning
- Returns the expected
[]types.FeeRunTxslice structure- Includes proper error handling and cleanup with
defer rows.Close()The implementation supports the transaction retrieval needs for the resend functionality mentioned in the PR objectives.
| if txErr = fp.db.CreateFee(ctx, dbTx, run.ID, fee); err != nil { | ||
| return fmt.Errorf("failed to create fee: %w", err) |
There was a problem hiding this comment.
Fix error variable reference.
There's a bug where err is used instead of txErr in the error return statement.
-if txErr = fp.db.CreateFee(ctx, dbTx, run.ID, fee); err != nil {
+if txErr = fp.db.CreateFee(ctx, dbTx, run.ID, fee); txErr != nil {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugin/fees/fees.go around lines 236 to 237, the error return statement
incorrectly references the variable 'err' instead of 'txErr'. Update the return
statement to use 'txErr' to correctly propagate the error from the CreateFee
call.
|
|
||
| // This is semi critical code as it could create a state mismatch between the verifier and the database. | ||
| if err = fp.db.SetFeeRunSuccess(ctx, run.ID); err != nil { | ||
| if txErr = fp.db.SetFeeRunSuccess(ctx, dbTx, run.ID); err != nil { |
There was a problem hiding this comment.
Fix variable usage in error check.
The error check is using the wrong variable - it should check txErr instead of err.
-if txErr = fp.db.SetFeeRunSuccess(ctx, dbTx, run.ID); err != nil {
+if txErr = fp.db.SetFeeRunSuccess(ctx, dbTx, run.ID); txErr != nil {📝 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 txErr = fp.db.SetFeeRunSuccess(ctx, dbTx, run.ID); err != nil { | |
| if txErr = fp.db.SetFeeRunSuccess(ctx, dbTx, run.ID); txErr != nil { |
🤖 Prompt for AI Agents
In plugin/fees/post_tx.go at line 160, the error check incorrectly uses the
variable 'err' instead of 'txErr'. Update the if condition to check 'txErr' for
errors after calling SetFeeRunSuccess to correctly handle the error returned by
that function.
| } | ||
| }() | ||
|
|
||
| if txdbErr = fp.db.SetFeeRunSent(ctx, dbTx, runId, txWithSig.Hash().Hex()); err != nil { //TODO pass the real tx id |
There was a problem hiding this comment.
Fix error assignment.
The error is being assigned to txdbErr but checked against err.
-if txdbErr = fp.db.SetFeeRunSent(ctx, dbTx, runId, txWithSig.Hash().Hex()); err != nil {
+if txdbErr = fp.db.SetFeeRunSent(ctx, dbTx, runId, txWithSig.Hash().Hex()); txdbErr != nil {🤖 Prompt for AI Agents
In plugin/fees/transaction.go at line 227, the error returned from SetFeeRunSent
is assigned to txdbErr but the condition incorrectly checks err. Change the
condition to check txdbErr instead of err to correctly handle the error returned
by SetFeeRunSent.
| if txdbErr = fp.db.CreateFeeRunTx(ctx, dbTx, runId, signedTxBytes, txWithSig.Hash().Hex(), 0, fp.config.ChainId); err != nil { | ||
| return fmt.Errorf("failed to create fee run tx: %w", err) | ||
| } |
There was a problem hiding this comment.
Fix error assignment.
Similar to above, the error is assigned to txdbErr but checked against err.
-if txdbErr = fp.db.CreateFeeRunTx(ctx, dbTx, runId, signedTxBytes, txWithSig.Hash().Hex(), 0, fp.config.ChainId); err != nil {
+if txdbErr = fp.db.CreateFeeRunTx(ctx, dbTx, runId, signedTxBytes, txWithSig.Hash().Hex(), 0, fp.config.ChainId); txdbErr != nil {📝 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 txdbErr = fp.db.CreateFeeRunTx(ctx, dbTx, runId, signedTxBytes, txWithSig.Hash().Hex(), 0, fp.config.ChainId); err != nil { | |
| return fmt.Errorf("failed to create fee run tx: %w", err) | |
| } | |
| if txdbErr = fp.db.CreateFeeRunTx(ctx, dbTx, runId, signedTxBytes, txWithSig.Hash().Hex(), 0, fp.config.ChainId); txdbErr != nil { | |
| return fmt.Errorf("failed to create fee run tx: %w", err) | |
| } |
🤖 Prompt for AI Agents
In plugin/fees/transaction.go around lines 231 to 233, the error returned from
CreateFeeRunTx is assigned to txdbErr but the code checks err instead. Fix this
by assigning the error to err instead of txdbErr or consistently use txdbErr for
both assignment and checking to ensure the error is properly handled.
| ) | ||
|
|
||
| func (p *PostgresBackend) CreateFeeRun(ctx context.Context, policyId uuid.UUID, state types.FeeRunState, fees ...verifierapi.FeeDto) (*types.FeeRun, error) { | ||
| func (p *PostgresBackend) CreateFeeRun(ctx context.Context, dbTx pgx.Tx, policyId uuid.UUID, state types.FeeRunState, fees ...verifierapi.FeeDto) (*types.FeeRun, error) { |
There was a problem hiding this comment.
Critical issue: Method signature updated but internal implementation still uses pool directly.
The method signature now accepts a pgx.Tx parameter but the implementation still creates its own transaction using p.pool.Begin(ctx) at line 30, completely ignoring the passed transaction parameter. This breaks the transactional consistency that the signature change was meant to provide.
Apply this diff to use the provided transaction parameter:
-func (p *PostgresBackend) CreateFeeRun(ctx context.Context, dbTx pgx.Tx, policyId uuid.UUID, state types.FeeRunState, fees ...verifierapi.FeeDto) (*types.FeeRun, error) {
+func (p *PostgresBackend) CreateFeeRun(ctx context.Context, dbTx pgx.Tx, policyId uuid.UUID, state types.FeeRunState, fees ...verifierapi.FeeDto) (*types.FeeRun, error) {
// Check policy id is valid
query := `select plugin_id from plugin_policies where id = $1`
- policyrows := p.pool.QueryRow(ctx, query, policyId)
+ policyrows := dbTx.QueryRow(ctx, query, policyId)
var pluginId string
err := policyrows.Scan(&pluginId)
if err != nil {
return nil, err
}
if pluginId != "vultisig-fees-feee" {
return nil, errors.New("plugin id not found or not vultisig-fees-feee")
}
- tx, err := p.pool.Begin(ctx)
- if err != nil {
- return nil, err
- }
- defer tx.Rollback(ctx)
runId := uuid.New()
- _, err = tx.Exec(ctx, `insert into fee_run (id, status, policy_id) values ($1, $2, $3) returning id`, runId, state, policyId)
+ _, err = dbTx.Exec(ctx, `insert into fee_run (id, status, policy_id) values ($1, $2, $3) returning id`, runId, state, policyId)
if err != nil {
return nil, fmt.Errorf("failed to insert fee run: %w", err)
}
for _, fee := range fees {
- _, err = tx.Exec(ctx, `insert into fee (id, fee_run_id, amount) values ($1, $2, $3)`, fee.ID, runId, fee.Amount)
+ _, err = dbTx.Exec(ctx, `insert into fee (id, fee_run_id, amount) values ($1, $2, $3)`, fee.ID, runId, fee.Amount)
if err != nil {
return nil, fmt.Errorf("failed to insert fee: %w", err)
}
}
- err = tx.Commit(ctx)
- if err != nil {
- return nil, fmt.Errorf("failed to commit transaction: %w", err)
- }
-
var run types.FeeRun
- err = p.pool.QueryRow(ctx, `select id, status, created_at, updated_at, tx_hash, policy_id, total_amount, fee_count from fee_run_with_totals where id = $1`, runId).Scan(&run.ID, &run.Status, &run.CreatedAt, &run.UpdatedAt, &run.TxHash, &run.PolicyID, &run.TotalAmount, &run.FeeCount)
+ err = dbTx.QueryRow(ctx, `select id, status, created_at, updated_at, tx_hash, policy_id, total_amount, fee_count from fee_run_with_totals where id = $1`, runId).Scan(&run.ID, &run.Status, &run.CreatedAt, &run.UpdatedAt, &run.TxHash, &run.PolicyID, &run.TotalAmount, &run.FeeCount)
if err != nil {
return nil, fmt.Errorf("failed to get fee run (post commit): %s", err)
}
return &run, nil
}📝 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.
| func (p *PostgresBackend) CreateFeeRun(ctx context.Context, dbTx pgx.Tx, policyId uuid.UUID, state types.FeeRunState, fees ...verifierapi.FeeDto) (*types.FeeRun, error) { | |
| func (p *PostgresBackend) CreateFeeRun(ctx context.Context, dbTx pgx.Tx, policyId uuid.UUID, state types.FeeRunState, fees ...verifierapi.FeeDto) (*types.FeeRun, error) { | |
| // Check policy id is valid | |
| query := `select plugin_id from plugin_policies where id = $1` | |
| policyrows := dbTx.QueryRow(ctx, query, policyId) | |
| var pluginId string | |
| if err := policyrows.Scan(&pluginId); err != nil { | |
| return nil, err | |
| } | |
| if pluginId != "vultisig-fees-feee" { | |
| return nil, errors.New("plugin id not found or not vultisig-fees-feee") | |
| } | |
| runId := uuid.New() | |
| _, err := dbTx.Exec(ctx, `insert into fee_run (id, status, policy_id) values ($1, $2, $3) returning id`, runId, state, policyId) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to insert fee run: %w", err) | |
| } | |
| for _, fee := range fees { | |
| _, err = dbTx.Exec(ctx, `insert into fee (id, fee_run_id, amount) values ($1, $2, $3)`, fee.ID, runId, fee.Amount) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to insert fee: %w", err) | |
| } | |
| } | |
| var run types.FeeRun | |
| err = dbTx.QueryRow(ctx, ` | |
| select id, status, created_at, updated_at, tx_hash, policy_id, total_amount, fee_count | |
| from fee_run_with_totals | |
| where id = $1 | |
| `, runId).Scan( | |
| &run.ID, | |
| &run.Status, | |
| &run.CreatedAt, | |
| &run.UpdatedAt, | |
| &run.TxHash, | |
| &run.PolicyID, | |
| &run.TotalAmount, | |
| &run.FeeCount, | |
| ) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get fee run (post commit): %w", err) | |
| } | |
| return &run, nil | |
| } |
🤖 Prompt for AI Agents
In storage/postgres/fees.go at line 17, the CreateFeeRun method signature now
accepts a pgx.Tx transaction parameter, but the implementation still starts a
new transaction using p.pool.Begin(ctx) around line 30. To fix this, remove the
call to p.pool.Begin(ctx) and use the provided dbTx transaction parameter
throughout the method to ensure transactional consistency as intended by the
updated signature.
e4ab853 to
15a008c
Compare
2b807cc to
4bb281b
Compare
1 participant
This adds dry run functionality, for testing locally without the need of sending txs, if nonces match on a not found tx it automatically resends
Summary by CodeRabbit
New Features
Improvements
Bug Fixes