feat: Add Developer Default Fee Configuration RPC Method #35
Conversation
…454-developer-suggestions-for-contract-and-method-fee
WalkthroughThe changes introduce a new fee configuration system for contract methods, including type definitions, a storage utility ( Changes
Sequence Diagram(s)sequenceDiagram
participant DApp
participant Snap
participant StateManager
DApp->>Snap: onRpcRequest({ method: "setDefaultFeeConfig", params })
Snap->>Snap: Validate params (contractAddress, methodName, config)
Snap->>StateManager: get(key)
alt Config exists
Snap-->>DApp: Return false (not overwritten)
else No config
Snap->>StateManager: set(key, config)
Snap-->>DApp: Return true (config set)
end
Estimated code review effort3 (~45 minutes) Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
yarn install v1.22.22 ✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/snap/src/index.tsx (1)
87-93: Fix switch clause variable declaration issue.The static analysis tool correctly identifies that variable declarations in switch clauses can be accessed by other cases, which can lead to bugs.
Apply this fix to wrap the declarations in a block:
case 'advanced_options': - // eslint-disable-next-line no-case-declarations - const currentStorageKey = await StateManager.get('currentStorageKey'); - // eslint-disable-next-line no-case-declarations - const persistedData = (await StateManager.get( - currentStorageKey, - )) as FeeConfigState; + { + const currentStorageKey = await StateManager.get('currentStorageKey'); + const persistedData = (await StateManager.get( + currentStorageKey, + )) as FeeConfigState; - await snap.request({ - method: 'snap_updateInterface', - params: { - id, - ui: <AdvancedOptionsForm values={persistedData || {}} />, - }, - }); - break; + await snap.request({ + method: 'snap_updateInterface', + params: { + id, + ui: <AdvancedOptionsForm values={persistedData || {}} />, + }, + }); + break; + }
🧹 Nitpick comments (3)
packages/snap/src/types/index.ts (1)
19-23: Consider refining the FeeConfigState type definition.The current intersection type might not enforce the intended constraint that all
FeeConfigkeys must be strings. The index signature[key: string]: stringcould potentially override the mapped type behavior.Consider this alternative approach for better type safety:
-export type FeeConfigState = { - [K in keyof FeeConfig]: string; -} & { - [key: string]: string; -}; +export type FeeConfigState = { + [K in keyof FeeConfig]-?: string; +} & Record<string, string>;This ensures all
FeeConfigproperties are required strings while maintaining extensibility.packages/snap/src/transactions/transaction.ts (1)
107-128: LGTM: Well-implemented fee configuration function.The
setDefaultFeeConfigfunction correctly:
- Validates required parameters
- Uses consistent storage key generation
- Respects existing configurations (won't overwrite)
- Provides clear return values
Consider these minor improvements:
export async function setDefaultFeeConfig( contractAddress: string, methodName: string, config: FeeConfig, ): Promise<boolean> { - if (!contractAddress || !methodName) { - throw new Error('Contract address and method name are required'); + if (!contractAddress) { + throw new Error('Contract address is required'); + } + if (!methodName) { + throw new Error('Method name is required'); } const storageKey = generateStorageKey(contractAddress, methodName); const existingConfig = await StateManager.get<FeeConfig>(storageKey); if (existingConfig) { return false; } - const defaultConfig = { - ...config, - }; - - await StateManager.set(storageKey, defaultConfig); + await StateManager.set(storageKey, config); return true; }packages/snap/src/index.tsx (1)
16-40: LGTM: Well-structured RPC handler with room for improvement.The RPC handler correctly implements the
setDefaultFeeConfigmethod with proper parameter validation and error handling.Consider these improvements for robustness:
export const onRpcRequest: OnRpcRequestHandler = async ({ request }) => { switch (request.method) { case 'setDefaultFeeConfig': { const { contractAddress, methodName, config } = request.params as { contractAddress: string; methodName: string; - config: Record<string, string>; + config: Record<string, string | undefined>; }; - if (!contractAddress || !methodName) { - throw new Error('Contract address and method name are required'); + if (!request.params || typeof request.params !== 'object') { + throw new Error('Invalid request parameters'); } const wasSet = await setDefaultFeeConfig( contractAddress, methodName, config as FeeConfig, ); return { success: wasSet }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/snap/snap.manifest.json(2 hunks)packages/snap/src/components/AdvancedOptionsForm.tsx(4 hunks)packages/snap/src/index.test.tsx(5 hunks)packages/snap/src/index.tsx(3 hunks)packages/snap/src/transactions/transaction.test.ts(3 hunks)packages/snap/src/transactions/transaction.ts(2 hunks)packages/snap/src/types/index.ts(1 hunks)
🧠 Learnings (7)
packages/snap/snap.manifest.json (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
packages/snap/src/transactions/transaction.ts (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
packages/snap/src/types/index.ts (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
packages/snap/src/components/AdvancedOptionsForm.tsx (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
packages/snap/src/transactions/transaction.test.ts (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
packages/snap/src/index.tsx (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
packages/snap/src/index.test.tsx (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
🧬 Code Graph Analysis (4)
packages/snap/src/transactions/transaction.ts (2)
packages/snap/src/types/index.ts (1)
FeeConfig(5-13)packages/snap/src/libs/StateManager.tsx (1)
StateManager(1-59)
packages/snap/src/components/AdvancedOptionsForm.tsx (1)
packages/snap/src/types/index.ts (1)
FeeConfigState(19-23)
packages/snap/src/transactions/transaction.test.ts (2)
packages/snap/src/libs/StateManager.tsx (1)
StateManager(1-59)packages/snap/src/transactions/transaction.ts (1)
setDefaultFeeConfig(107-128)
packages/snap/src/index.tsx (2)
packages/snap/src/transactions/transaction.ts (1)
setDefaultFeeConfig(107-128)packages/snap/src/types/index.ts (2)
FeeConfig(5-13)FeeConfigState(19-23)
🪛 Biome (2.1.2)
packages/snap/src/index.tsx
[error] 91-93: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🧰 Additional context used
🧠 Learnings (7)
packages/snap/snap.manifest.json (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
packages/snap/src/transactions/transaction.ts (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
packages/snap/src/types/index.ts (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
packages/snap/src/components/AdvancedOptionsForm.tsx (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
packages/snap/src/transactions/transaction.test.ts (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
packages/snap/src/index.tsx (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
packages/snap/src/index.test.tsx (1)
Learnt from: epsjunior
PR: #34
File: packages/snap/src/transactions/transaction.ts:15-15
Timestamp: 2025-07-22T05:49:18.238Z
Learning: In packages/snap/src/transactions/transaction.ts, the team has decided to keep the ABI hardcoded to chains.localnet.consensusMainContract?.abi rather than making it configurable. This was a deliberate architectural decision made after discussion.
🧬 Code Graph Analysis (4)
packages/snap/src/transactions/transaction.ts (2)
packages/snap/src/types/index.ts (1)
FeeConfig(5-13)packages/snap/src/libs/StateManager.tsx (1)
StateManager(1-59)
packages/snap/src/components/AdvancedOptionsForm.tsx (1)
packages/snap/src/types/index.ts (1)
FeeConfigState(19-23)
packages/snap/src/transactions/transaction.test.ts (2)
packages/snap/src/libs/StateManager.tsx (1)
StateManager(1-59)packages/snap/src/transactions/transaction.ts (1)
setDefaultFeeConfig(107-128)
packages/snap/src/index.tsx (2)
packages/snap/src/transactions/transaction.ts (1)
setDefaultFeeConfig(107-128)packages/snap/src/types/index.ts (2)
FeeConfig(5-13)FeeConfigState(19-23)
🪛 Biome (2.1.2)
packages/snap/src/index.tsx
[error] 91-93: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (14)
packages/snap/snap.manifest.json (2)
10-10: LGTM: Source hash updated for new bundle.The shasum update is expected when adding new functionality to the snap bundle.
24-27: LGTM: RPC permission correctly configured.The
endowment:rpcpermission is properly configured to allow dapps to call RPC methods while restricting access from other snaps, which aligns with security best practices.packages/snap/src/types/index.ts (1)
5-13: LGTM: Well-structured base fee configuration type.The
FeeConfigtype properly defines optional fee parameters with appropriate naming conventions and includes an index signature for extensibility.packages/snap/src/transactions/transaction.ts (1)
5-6: LGTM: Proper imports for new functionality.The imports for
StateManagerandFeeConfigare correctly added to support the new fee configuration feature.packages/snap/src/components/AdvancedOptionsForm.tsx (3)
14-14: LGTM: Improved type consistency with centralized definitions.Importing
FeeConfigStatefrom the types module eliminates duplicate type definitions and ensures consistency across components.
17-17: LGTM: Consistent type usage in props.Using
FeeConfigStatefor thevaluesprop aligns with the new centralized type definitions.
23-23: LGTM: Improved default value handling with nullish coalescing.Changing from
||to??is more semantically correct for handlingnullandundefinedvalues, preventing issues with falsy strings like"0".Also applies to: 32-32, 67-67, 76-76, 84-84, 92-92, 99-99
packages/snap/src/index.tsx (2)
2-2: LGTM: Proper imports for new RPC functionality.The imports correctly include the new
OnRpcRequestHandlertype and related functions/types needed for the RPC feature.Also applies to: 10-14
93-93: LGTM: Updated type casting for new fee configuration types.The type casting to
FeeConfigStateandFeeConfigcorrectly aligns with the new centralized type definitions.Also applies to: 114-114
packages/snap/src/transactions/transaction.test.ts (2)
347-468: Excellent comprehensive test coverage forsetDefaultFeeConfig.The test suite thoroughly covers all the key scenarios:
- ✅ Successful configuration setting when none exists
- ✅ Preservation of existing configurations (no overwrite)
- ✅ Contract address normalization to lowercase
- ✅ Error handling for missing required parameters
- ✅ Partial configuration support
The mocking strategy is consistent and the test assertions accurately verify the expected behavior based on the implementation.
34-41: Well-structured mock setup with proper typing.The StateManager mock is comprehensive and the typed mock variables enhance type safety and developer experience during testing.
Also applies to: 48-50
packages/snap/src/index.test.tsx (3)
76-210: Comprehensive and well-structured RPC handler test coverage.The test suite for
onRpcRequestexcellently covers all scenarios:
- ✅ Successful fee configuration setting
- ✅ Handling existing configurations
- ✅ Input validation for required parameters
- ✅ Missing configuration parameter handling
- ✅ Error propagation from underlying functions
- ✅ Unknown method error handling
The test assertions properly verify both the function calls and return values, ensuring the RPC handler behaves correctly in all scenarios.
32-56: Excellent test infrastructure improvements.The enhanced mock setup provides:
- ✅ Explicit and properly typed mocks for better IntelliSense
- ✅ Consistent test cleanup with
beforeEachhook- ✅ Better separation of concerns in mock configuration
These changes significantly improve test maintainability and developer experience.
Also applies to: 72-74
238-359: Well-refactored user input tests with improved coverage.The updated tests provide:
- ✅ Cleaner event type handling with string literals
- ✅ Comprehensive coverage of user interaction scenarios
- ✅ Proper mock verification for StateManager operations
- ✅ Appropriate handling of unknown user actions
The refactoring enhances test readability and ensures robust validation of user input handling.
| } | ||
| const storageKey = generateStorageKey(contractAddress, methodName); | ||
|
|
||
| const existingConfig = await StateManager.get<FeeConfig>(storageKey); |
There was a problem hiding this comment.
@epsjunior why can we update the config if it was previously set?
I think that we are mixing the user catched fees and the developer suggestion. We always can accept and record the developer's fee suggestion.
Then, if the user wants to set their own, that's good too.
At the moment of sending the transaction, the priority would be:
- User fees configuration
- Developer fees configuration
Add Developer Default Fee Configuration RPC Method
Summary
Introduces a new RPC method to allow developers to set default fee configurations for specific contract and method combinations in the GenLayer Wallet snap. This enhancement enables dApps and contract developers to provide sensible fee parameter defaults, improving the onboarding and transaction experience for users. The system ensures user preferences always take precedence, and developer defaults are only used when no user configuration exists.
Changes
🚀 New Features
setDefaultFeeConfigRPC method for setting per-contract+method default fee parameterstransfer,approve)FeeConfigandFeeConfigStatetypes for strong typing of fee configurations🎯 Problem Solved
Issue: Users previously had to manually set all fee parameters, and there was no way for dApps to suggest sensible defaults for new contracts or methods.
Solution:
🔧 Implementation Details
onRpcRequesthandler forsetDefaultFeeConfigcontractAddress_methodName) to store defaults📁 File Structure
🏗️ Usage
Set Developer Default Fee Config (RPC)
Priority System
🧪 Testing
✨ Code Quality
transactionsmodule🛡️ Security & Audit
endowment:rpcpermission is usedTesting: ✅ All tests pass, including new RPC and priority logic
Type Safety: ✅ Full TypeScript coverage
User Experience: ✅ Users always have control, but benefit from sensible defaults
Developer Experience: ✅ Easy to set per-method defaults via a single RPC call
Security: ✅ User data protected, no breaking changes, clear error handling
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores