This repository was archived by the owner on Feb 8, 2026. It is now read-only.
⚡️ Adding more transaction based functionality to fees#120
Merged
Conversation
Contributor
WalkthroughThe changes modify an error log message capitalization in plugin API, remove the entire Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant FeePlugin
participant EthereumSDK
participant Database
Client->>API: CreatePluginPolicy (with policy)
API->>FeePlugin: Validate policy
FeePlugin-->>API: Validation error (detailed message)
API-->>Client: HTTP 400 with detailed error
Client->>FeePlugin: Propose Transactions
FeePlugin->>FeePlugin: ValidateProposedTransactions
FeePlugin->>Database: Get recipe
FeePlugin->>FeePlugin: Evaluate transactions
FeePlugin-->>Client: Error if disallowed
FeePlugin->>FeePlugin: HandleCollections
FeePlugin->>FeePlugin: executeFeeCollection (sequentially)
FeePlugin->>FeePlugin: initSign (with runId)
FeePlugin->>Database: Mark fee run as sent
FeePlugin->>FeePlugin: SigningComplete
FeePlugin->>EthereumSDK: Broadcast signed transaction
EthereumSDK-->>FeePlugin: Result or error
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous 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). (2)
✨ Finishing Touches
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. 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 (
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the fees plugin by introducing transaction-based workflows, including DB updates on signing, policy-driven validation, and broadcast support via a new EVM SDK.
- Add
runIdtoinitSignto updatefee_runstatus andtx_idin the database - Implement
ValidateProposedTransactionsandSigningCompletefor policy validation and transaction broadcast - Integrate
recipes/sdk/evmSDK, removetssstubs anderrgroupconcurrency in fee handling
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| plugin/fees/transaction.go | Added runId handling, fixed error prefix, and new methods for validation and broadcast |
| plugin/fees/fees.go | Integrated EVM SDK initialization, removed tss & errgroup, updated handler signature |
| plugin/common/common.go | Prefixed plugincommon in errors and switched hex encoding to Text(16) |
| api/plugin.go | Included error details in client response for policy validation |
Comments suppressed due to low confidence (5)
plugin/fees/transaction.go:306
- [nitpick] Exported method ValidateProposedTransactions lacks a doc comment. Consider adding a brief description of its purpose and parameters.
func (fp *FeePlugin) ValidateProposedTransactions(policy vtypes.PluginPolicy, txs []vtypes.PluginKeysignRequest) error {
plugin/common/common.go:142
- The GasPrice field in createAccessListArgs is defined but never populated. Consider removing it or populating it to avoid confusion.
}
plugin/fees/transaction.go:352
- Missing import for the tss package in this file. The SigningComplete method references tss.KeysignResponse but tss is not imported, causing a compilation error.
func (fp *FeePlugin) SigningComplete(ctx context.Context, signature tss.KeysignResponse, signRequest vtypes.PluginKeysignRequest, policy vtypes.PluginPolicy) error {
plugin/fees/transaction.go:310
- Use the %w verb instead of %v when wrapping errors to preserve the original error for unwrapping (e.g., fmt.Errorf("...: %w", err)).
return fmt.Errorf("failed to validate plugin policy: %v", err)
api/plugin.go:184
- This exposes internal error details in the API response, which may leak sensitive information. Consider logging the error internally and returning a more generic message to the client.
return c.JSON(http.StatusBadRequest, NewErrorResponse(fmt.Sprintf("failed to validate policy: %s", err)))
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/fees/fees.go (1)
286-288: Consider the performance impact of removing concurrent execution.The change from concurrent (errgroup) to sequential execution could significantly slow down fee collection when processing multiple transactions. Was this change intentional?
If performance is a concern, consider maintaining concurrent execution with proper error handling:
-for _, keySignRequest := range keySignRequests { - req := keySignRequest - if err := fp.initSign(ctx, req, feePolicy, feeRun.ID); err != nil { - return fmt.Errorf("failed to init sign: %w", err) - } -} +var eg errgroup.Group +for _, keySignRequest := range keySignRequests { + req := keySignRequest + eg.Go(func() error { + return fp.initSign(ctx, req, feePolicy, feeRun.ID) + }) +} +if err := eg.Wait(); err != nil { + return fmt.Errorf("failed to init sign: %w", err) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/plugin.go(1 hunks)plugin/common/common.go(5 hunks)plugin/fees/fees.go(6 hunks)plugin/fees/transaction.go(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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: 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: 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.
api/plugin.go (2)
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.
plugin/common/common.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: 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: 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: 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: 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.
plugin/fees/transaction.go (7)
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.
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: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: 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:178-183
Timestamp: 2025-06-18T18:28:19.759Z
Learning: In the payroll plugin, the Hash field in PluginKeysignRequest is intentionally set to the unsigned transaction hex (same as Message field) because computing a hash with empty V,R,S signature fields doesn't make sense, and it's documented as "not on-chain hash without signature". This is a deliberate placeholder approach.
plugin/fees/fees.go (7)
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.
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: 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: 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.
⏰ 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 (8)
api/plugin.go (1)
184-184: Good improvement to error messaging!Including the specific validation error details provides better debugging information for API consumers.
plugin/common/common.go (2)
47-47: Good improvement to error message prefixes.Changing from "p." to "plugincommon." makes error sources more explicit and easier to trace.
Also applies to: 103-103, 151-151, 161-161
200-202: Hex serialization is valid withText(16)and0xprefix.big.Int.Text(16)produces the minimal lowercase hex string (no leading zeros), and you’re manually prepending0xjust asBytes2Hexdid. This matches Ethereum JSON-RPC expectations, so no changes are needed.plugin/fees/transaction.go (3)
143-143: Fixed error message capitalization.Good consistency improvement.
306-350: Well-implemented transaction validation logic.The method properly validates transactions against the policy using the recipe engine, with good error handling and descriptive error messages.
352-369: Good implementation of transaction broadcasting.The method correctly handles signature components and provides appropriate logging. The error handling with detailed context is helpful for debugging.
plugin/fees/fees.go (2)
69-72: Proper Ethereum SDK initialization.Good addition of the Ethereum SDK for transaction broadcasting support.
107-112: Helpful documentation added.The comment clearly explains the three fee collection modes, improving code maintainability.
webpiratt
suggested changes
Jul 7, 2025
Contributor
There was a problem hiding this comment.
I suggest to replace local func EvmMakeUnsignedTransfer (and funcs used inside) with sdk.MakeAnyTransfer it has same args and same implementation inside, and also then you don't need rpcClient in FeePlugin structure at all. And also then FeePlugin code would be cleaned up from any tx build funcs
Everything rest looks good to me
johnnyluo
suggested changes
Jul 7, 2025
added 5 commits
July 9, 2025 11:18
johnnyluo
reviewed
Jul 9, 2025
|
|
||
| if err := s.plugin.ValidatePluginPolicy(policy); err != nil { | ||
| s.logger.WithError(err).Error("Failed to validate plugin policy") | ||
| s.logger.WithError(err).Error("failed to validate plugin policy: ", err) |
Contributor
There was a problem hiding this comment.
@garry-sharp err already added with WithError(err)
johnnyluo
reviewed
Jul 9, 2025
| if err != nil { | ||
| return fmt.Errorf("failed to parse tx indexer id: %w", err) | ||
| } | ||
| _, err = fp.db.Pool().Exec(ctx, "UPDATE fee_run SET status = 'sent', tx_id = $1 WHERE id = $2", txId, runId) |
Contributor
There was a problem hiding this comment.
shouldn't you put this somewhere in the db folder?
webpiratt
reviewed
Jul 9, 2025
webpiratt
reviewed
Jul 9, 2025
webpiratt
reviewed
Jul 9, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores