fix: update return types in database and policy methods to use pointe…#121
Conversation
…rs for better error handling
WalkthroughThis set of changes standardizes the handling of the Changes
Sequence Diagram(s)sequenceDiagram
participant API
participant Service
participant Storage
API->>Service: GetPluginPolicy(policyID)
Service->>Storage: GetPluginPolicy(policyID)
Storage-->>Service: *PluginPolicy
Service-->>API: *PluginPolicy
API->>Service: DeletePluginPolicyById(policy)
Service->>API: verifyPolicySignature(*policy)
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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). (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates database and service methods to return pointers for improved error handling, and adjusts all callers accordingly.
- Changed
GetPluginPolicyandCreateFeeRunsignatures in storage and interface to return pointers. - Updated service layer and API/plugin code to handle new pointer return types.
- Simplified service implementations by delegating directly to the database calls.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/postgres/policy.go | Changed GetPluginPolicy to return *PluginPolicy and updated returns. |
| storage/postgres/fees.go | Changed CreateFeeRun to return *FeeRun, added defer tx.Rollback, and updated returns. |
| storage/db.go | Updated DatabaseStorage interface method signatures to use pointers. |
| service/policy.go | Updated service interface and methods to return pointers and delegate directly. |
| plugin/payroll/transaction.go | Updated calls to dereference *PluginPolicy when calling methods. |
| plugin/fees/fees.go | Updated call to dereference *PluginPolicy and adjusted import grouping. |
| api/plugin.go | Updated calls to dereference *PluginPolicy for validation and signature checks. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
storage/postgres/fees.go (1)
35-35: Standardize error formatting for consistency.The error messages use both
%wand%sformatters inconsistently. For better error handling and debugging, consider using%wfor error wrapping consistently.- return nil, fmt.Errorf("failed to insert fee: %s", err) + return nil, fmt.Errorf("failed to insert fee: %w", err)- return nil, fmt.Errorf("failed to commit transaction: %s", err) + return nil, fmt.Errorf("failed to commit transaction: %w", err)- return nil, fmt.Errorf("failed to get fee run (post commit): %s", err) + return nil, fmt.Errorf("failed to get fee run (post commit): %w", err)Also applies to: 41-41, 47-47, 53-53
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/plugin.go(2 hunks)plugin/fees/fees.go(3 hunks)plugin/payroll/transaction.go(3 hunks)service/policy.go(2 hunks)storage/db.go(2 hunks)storage/postgres/fees.go(1 hunks)storage/postgres/policy.go(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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: 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.
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.
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.
plugin/payroll/transaction.go (6)
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.
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.
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.
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: 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.
api/plugin.go (3)
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.
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: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.
storage/postgres/policy.go (2)
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.
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.
storage/postgres/fees.go (1)
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.
service/policy.go (1)
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.
storage/db.go (2)
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.
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.
plugin/fees/fees.go (6)
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: 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.
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.
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.
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.
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.
🧬 Code Graph Analysis (3)
api/plugin.go (1)
internal/types/keysign.go (1)
PluginKeysignRequest(7-12)
storage/postgres/policy.go (1)
storage/postgres/db.go (1)
PostgresBackend(15-17)
storage/db.go (2)
internal/types/fees.go (2)
FeeRunState(11-11)FeeRun(27-36)internal/verifierapi/fees.go (1)
FeeDto(11-20)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (19)
plugin/fees/fees.go (3)
19-19: LGTM: Import reordering for clarity.The vault import reordering improves code organization without functional changes.
165-165: LGTM: Correct pointer dereferencing.The change from
policyto*policycorrectly dereferences the pointer returned byGetPluginPolicyto pass the value toexecuteFeeCollection, which expects avtypes.PluginPolicyvalue rather than a pointer.
233-233: LGTM: Minor comment formatting improvement.The formatting adjustment improves comment readability.
api/plugin.go (2)
62-62: LGTM: Correct pointer dereferencing for validation.The change from
policyto*policycorrectly dereferences the pointer returned byGetPluginPolicybefore passing it toValidateProposedTransactions, which expects avtypes.PluginPolicyvalue.
264-264: LGTM: Correct pointer dereferencing for signature verification.The change from
policyto*policycorrectly dereferences the pointer returned byGetPluginPolicybefore passing it toverifyPolicySignature, maintaining consistency with the updated return types.plugin/payroll/transaction.go (3)
22-25: LGTM: Import reordering for clarity.The import statement reordering improves code organization without affecting functionality.
62-62: LGTM: Correct pointer dereferencing for transaction proposal.The change from
pluginPolicyto*pluginPolicycorrectly dereferences the pointer returned byGetPluginPolicybefore passing it toProposeTransactions, which expects avtypes.PluginPolicyvalue.
72-72: LGTM: Correct pointer dereferencing for signing initialization.The change from
pluginPolicyto*pluginPolicycorrectly dereferences the pointer before passing it toinitSign, maintaining consistency with the updated return types.storage/postgres/policy.go (3)
14-14: LGTM: Method signature updated to return pointer.The method signature change from
vtypes.PluginPolicyto*vtypes.PluginPolicyenables better error handling by distinguishing between "not found" (nil) and "found but empty" (zero value).
16-16: LGTM: Consistent nil returns for error cases.Returning
nilinstead of zero values on errors aligns with the pointer return type and provides clearer error semantics.Also applies to: 36-36
38-38: LGTM: Safe return of local variable address.Returning
&policyis safe here because thepolicyvariable contains data copied during the database scan operation, not a reference to external memory.service/policy.go (3)
21-21: LGTM: Interface updated to return pointer.The interface method signature change to return
*vtypes.PluginPolicymaintains consistency with the storage layer changes and enables better error handling.
129-129: LGTM: Simplified implementation.Directly returning the database result eliminates unnecessary intermediate error handling while maintaining the same error propagation behavior.
132-134: LGTM: Consistent pointer return implementation.The simplified implementation directly returns the pointer from the database layer, maintaining consistency with the updated return types and eliminating unnecessary error wrapping.
storage/postgres/fees.go (3)
14-14: LGTM: Function signature update improves error handling.The change from returning a value type to a pointer type aligns with the PR objective of improving error handling and is consistent with the interface definition in
storage/db.go.
29-31: LGTM: Improved transaction handling with deferred rollback.The addition of
defer tx.Rollback(ctx)is a good practice that ensures the transaction is rolled back on early returns or panics. The rollback will be a no-op if the transaction has already been committed.
56-56: LGTM: Return statement correctly returns pointer.The change from
return run, niltoreturn &run, nilis consistent with the updated function signature and provides better error handling semantics.storage/db.go (2)
18-18: LGTM: Interface method signature update is consistent.The change from
(vtypes.PluginPolicy, error)to(*vtypes.PluginPolicy, error)aligns with the implementation changes and improves error handling by allowing nil returns on errors.
33-33: LGTM: Interface method signature update matches implementation.The change from
(types.FeeRun, error)to(*types.FeeRun, error)is consistent with the implementation instorage/postgres/fees.goand follows the same pattern as other pointer return types in the interface.
…rs for better error handling
Summary by CodeRabbit
No changes to user-facing features or functionality.