feat: store fees config for contractmethod#34
Conversation
WalkthroughA new transaction utility module was added to handle GenLayer transaction parsing and storage key generation. State management logic was updated to use transaction-specific storage keys instead of simple address-based keys. Tests and dependencies were updated to reflect these changes, and the StateManager's Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SnapHandlers
participant StateManager
participant TransactionUtils
User->>SnapHandlers: onTransaction(transaction)
SnapHandlers->>TransactionUtils: getTransactionStorageKey(transaction)
TransactionUtils-->>SnapHandlers: storageKey
SnapHandlers->>StateManager: set(storageKey, data)
Note over SnapHandlers: Use storageKey for all state operations
User->>SnapHandlers: onUserInput(event)
SnapHandlers->>TransactionUtils: getTransactionStorageKey(transaction)
TransactionUtils-->>SnapHandlers: storageKey
SnapHandlers->>StateManager: get(storageKey)
StateManager-->>SnapHandlers: data or null
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 📜 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). (3)
✨ 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: 3
🧹 Nitpick comments (3)
packages/snap/src/index.tsx (1)
32-34: Consider extracting the storage key retrieval pattern.The pattern of getting
currentStorageKeyand then using it to retrieve persisted data is repeated multiple times. Consider extracting this into a helper function to reduce duplication and improve maintainability.Example helper function:
async function getPersistedData<T = any>(): Promise<T | null> { const currentStorageKey = await StateManager.get('currentStorageKey'); if (!currentStorageKey) return null; return await StateManager.get(currentStorageKey); }This would simplify the repeated pattern and provide consistent error handling for missing storage keys.
Also applies to: 59-63, 83-85
packages/snap/src/transactions/transaction.ts (2)
30-32: Simplify the conditional logic for default values.The ternary operators can be simplified using logical OR operators for better readability.
const result = { - contractAddress: contractAddress ? contractAddress : 'default', - methodName: methodName ? methodName : 'unknown' + contractAddress: contractAddress || 'default', + methodName: methodName || 'unknown' };
36-43: Consider logging errors for debugging purposes.The catch block silently swallows all errors, which could make debugging difficult in production.
Consider adding optional error logging:
} catch (error) { + // Log error for debugging purposes + console.warn('Failed to parse GenLayer transaction:', error); const result = { contractAddress: 'default', methodName: 'unknown' };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
packages/snap/package.json(1 hunks)packages/snap/snap.manifest.json(1 hunks)packages/snap/src/index.test.tsx(8 hunks)packages/snap/src/index.tsx(5 hunks)packages/snap/src/libs/StateManager.tsx(1 hunks)packages/snap/src/transactions/transaction.test.ts(1 hunks)packages/snap/src/transactions/transaction.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/snap/src/index.test.tsx (3)
packages/snap/src/transactions/transaction.ts (1)
getTransactionStorageKey(74-79)packages/snap/src/libs/StateManager.tsx (1)
StateManager(1-59)packages/snap/src/index.tsx (1)
onTransaction(12-25)
packages/snap/src/transactions/transaction.test.ts (1)
packages/snap/src/transactions/transaction.ts (4)
parseGenLayerTransaction(13-44)extractMethodSelector(51-54)generateStorageKey(62-67)getTransactionStorageKey(74-79)
🪛 Biome (1.9.4)
packages/snap/src/index.tsx
[error] 59-59: 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:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 61-63: 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:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (21)
packages/snap/package.json (1)
32-32: [email protected] is current and has no known vulnerabilities
- Verified that 0.11.2 is the latest documented release as of June 2025 (no newer versions published on npm).
- Checked Snyk, CVE, and npm advisories—no security issues reported for 0.11.2.
No further action needed.
packages/snap/snap.manifest.json (1)
10-10: LGTM - Expected checksum update.The SHA checksum update correctly reflects the new source code changes including the transaction utilities and updated dependencies.
packages/snap/src/libs/StateManager.tsx (1)
9-9: LGTM - Improved API consistency.Returning
nullinstead ofundefinedfor missing keys provides more consistent and predictable behavior. This aligns well with the updated usage patterns in the calling code.packages/snap/src/index.tsx (2)
10-10: LGTM - Good addition of transaction utility import.The import of
getTransactionStorageKeysupports the new transaction-specific storage approach, improving state isolation between different transaction types.
12-25: onTransaction data handling verifiedThe
getTransactionStorageKeyimplementation is backed by comprehensive tests inpackages/snap/src/transactions/transaction.test.ts, which cover:
- Generating keys via GenLayer parsing
- Fallback to
"default_unknown"when parsing fails- Handling transactions without a
datafield- Handling completely empty transaction objects
No further changes are required here.
packages/snap/src/index.test.tsx (3)
7-7: LGTM - Proper test setup for new transaction utilities.The import and mocking of the transaction utility module correctly supports testing the new storage key functionality.
Also applies to: 10-10
27-45: LGTM - Comprehensive test coverage for transaction handler changes.The test properly verifies:
- Transaction data including the
datafield for method parsing- Mock return value from
getTransactionStorageKey- Correct calls to state management with the new storage key pattern
- Expected interface creation flow
The test data with
data: '0xa9059cbb00000000'provides realistic transaction data for testing the parsing functionality.
48-85: LGTM - Updated user input tests align with implementation changes.The tests correctly update the mock implementations to:
- Return the mock storage key when
'currentStorageKey'is requested- Return test data when the mock storage key is used
- Verify the correct sequence of state operations
The test coverage maintains the same level of verification while adapting to the new storage key approach.
Also applies to: 87-133
packages/snap/src/transactions/transaction.ts (4)
18-19: Verify the assumption about args[1] containing the contract address.The code assumes that
parsed.args[1]contains the contract address without validation. This could be fragile if the ABI structure changes.Please verify that the consensus contract ABI consistently places the contract address at
args[1]. Consider adding validation:// Extract contract address from parsed.args[1] - const contractAddress = parsed?.args[1]; + const contractAddress = parsed?.args?.[1]; + if (contractAddress && typeof contractAddress !== 'string') { + throw new Error('Invalid contract address format'); + }
51-54: LGTM: Clean wrapper function.The
extractMethodSelectorfunction is well-implemented as a focused wrapper around the parsing logic.
62-67: LGTM: Robust key generation with proper normalization.The function correctly handles null/undefined addresses and normalizes the address to lowercase, ensuring consistent storage key format.
74-79: LGTM: Good integration of parsing and key generation.The function properly handles missing transaction data by providing an empty string fallback, maintaining the error-resilient design pattern.
packages/snap/src/transactions/transaction.test.ts (9)
3-24: LGTM: Proper mock setup for external dependencies.The mocking strategy correctly isolates the unit under test by mocking both
genlayer-jsandethersdependencies with appropriate structure.
30-56: LGTM: Comprehensive test for valid transaction parsing.The test properly sets up all mock dependencies and validates the expected parsing behavior with realistic data.
57-69: LGTM: Good error handling test coverage.The test ensures that parsing failures are handled gracefully by returning default values.
71-95: LGTM: Edge case testing for null contract address.The test validates that the function handles null contract addresses appropriately while still extracting valid method names.
97-122: LGTM: Edge case testing for invalid method names.The test ensures proper handling when the method extraction fails but contract address is valid.
124-178: LGTM: Thorough testing of method extraction wrapper.The test suite properly validates both the successful extraction and error handling scenarios for the
extractMethodSelectorfunction.
180-208: LGTM: Complete coverage of storage key generation scenarios.The tests validate address normalization, default handling for undefined/empty addresses, and proper key format generation.
210-304: LGTM: Comprehensive integration testing.The test suite covers the complete integration flow from transaction object to storage key, including various error scenarios and edge cases like missing data properties.
229-231: Good test documentation: Transaction.to field is ignored.The comment on line 230 clearly documents that the
tofield is now ignored in favor of parsed contract address, which helps clarify the behavioral change from the previous implementation.
epsjunior
changed the title
cristiam86
left a comment
cristiam86
left a comment
There was a problem hiding this comment.
@epsjunior please fix test case
2 participants
Store Fees Configuration Per Contract+Method Combination
Summary
Enhances the GenLayer wallet snap to store fees configuration per contract+method combination instead of just per contract address. This improvement allows different methods within the same contract to have separate fee configurations, providing more granular control and better user experience. The implementation includes GenLayer transaction parsing, composite storage key generation, and comprehensive test coverage.
Changes
🚀 New Features
contractAddress_methodName(e.g.,0x123...abc_transfer)transfer,approve,mintinstead of hex selectorsdefault_unknown) when parsing fails🔧 Core Implementation
parsed.args[1]instead oftransaction.tofor actual contract addressgenlayer-jsandethersfor proper transaction decoding🏗️ Architecture Updates
📁 File Structure
🔍 Transaction Parsing Logic
🎯 GenLayer Integration Details
chains.localnet.consensusMainContract.abiparsed.args[4]transaction dataabi.calldata.decode()for method name extraction🧪 Testing Coverage
Transaction Parsing Tests (
transactions/transaction.test.ts):args[1]Handler Tests (
index.test.tsx):Mock Infrastructure:
genlayer-jsandethersdependencies🔄 State Management Flow
📋 Storage Key Examples
🛠️ Implementation Benefits
🔧 Error Handling
default_unknownstorage key🎨 Code Quality Improvements
utilsfolder totransactionsfor better semantics🔄 Migration Impact
✨ Technical Implementation
🎯 User Experience
🔗 Dependencies
Testing: ✅ All tests pass with comprehensive coverage
Type Safety: ✅ Full TypeScript integration with proper error handling
Documentation: ✅ Clear JSDoc comments and inline documentation
Architecture: ✅ Clean separation with dedicated transaction handling
User Experience: ✅ Seamless upgrade with enhanced functionality
Production Ready: ✅ No debug logs, clean professional codebase
GenLayer Integration: ✅ Proper transaction parsing using official libraries
Summary by CodeRabbit
Summary by CodeRabbit