Release 23.05.2025 #215
Release 23.05.2025 #215
Conversation
vignesha22
requested review from
ch4r10t33r,
cryptonoyaiba,
kanthgithub and
nikhilkumar1612
WalkthroughThis update introduces a new environment variable for chain-specific legacy transaction enforcement, refactors the Paymaster class to use named constructor parameters, and adjusts transaction fee logic based on chain IDs. It also removes the Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin/API
participant Server as Server
participant Paymaster as Paymaster
participant Blockchain as Blockchain
Admin->>Server: Request action (e.g., deposit, whitelist)
Server->>Paymaster: Instantiate with config (including skipType2Txns)
Paymaster->>Paymaster: Check if chainId in skipType2Txns
alt Legacy transaction required
Paymaster->>Blockchain: Send legacy transaction (gasPrice)
else EIP-1559 transaction
Paymaster->>Blockchain: Send EIP-1559 transaction (maxFeePerGas, etc.)
end
Blockchain-->>Paymaster: Transaction result
Paymaster-->>Server: Result
Server-->>Admin: Response
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. ✨ 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
backend/.env.example(1 hunks)backend/CHANGELOG.md(1 hunks)backend/migrations/2025052200001-update-sponsorship-table.cjs(1 hunks)backend/package.json(1 hunks)backend/src/models/sponsorship-policy.ts(0 hunks)backend/src/paymaster/index.ts(7 hunks)backend/src/plugins/config.ts(3 hunks)backend/src/repository/sponsorship-policy-repository.ts(0 hunks)backend/src/routes/admin-routes.ts(1 hunks)backend/src/routes/deposit-route.ts(3 hunks)backend/src/routes/paymaster-routes.ts(6 hunks)backend/src/routes/token-routes.ts(0 hunks)backend/src/routes/whitelist-routes.ts(1 hunks)backend/src/server.ts(1 hunks)backend/src/types/sponsorship-policy-dto.ts(0 hunks)
💤 Files with no reviewable changes (4)
- backend/src/routes/token-routes.ts
- backend/src/types/sponsorship-policy-dto.ts
- backend/src/models/sponsorship-policy.ts
- backend/src/repository/sponsorship-policy-repository.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/src/server.ts (1)
backend/src/paymaster/index.ts (1)
Paymaster(67-1445)
backend/src/routes/whitelist-routes.ts (1)
backend/src/paymaster/index.ts (1)
Paymaster(67-1445)
backend/src/routes/admin-routes.ts (1)
backend/src/paymaster/index.ts (1)
Paymaster(67-1445)
🪛 markdownlint-cli2 (0.17.2)
backend/CHANGELOG.md
2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
4-4: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (17)
backend/.env.example (1)
15-16: LGTM! Well-documented environment variable addition.The new environment variable is clearly documented with an appropriate comment explaining the expected format (comma-separated chain IDs) and includes a reasonable example value.
backend/src/plugins/config.ts (3)
45-45: LGTM! Consistent schema definition.The new configuration property is properly defined as an optional array of strings, which aligns with the expected comma-separated chain ID format.
92-92: LGTM! Proper environment variable parsing.The parsing logic correctly handles the comma-separated string format and provides a sensible default (empty array) when the environment variable is undefined.
135-135: LGTM! Consistent configuration handling.The final configuration object maintains the same parsing logic as the validation object, ensuring consistency throughout the configuration system.
backend/migrations/2025052200001-update-sponsorship-table.cjs (2)
16-23: LGTM! Proper down migration implementation.The down migration correctly restores the
IS_PUBLICcolumn with the same data type and constraints, ensuring the migration is reversible.
5-12:Details
❓ Verification inconclusive
Verify removeColumn syntax and data type choice.
The
removeColumncall appears to include a column definition object as the third parameter, which is typically not required for column removal. Most SequelizeremoveColumncalls only need the table identifier and column name.Additionally,
DataTypes.TEXTseems unusual for a field namedIS_PUBLIC, which would typically be boolean. However, this might be intentional for legacy compatibility.Please verify this migration syntax by running:
🏁 Script executed:
#!/bin/bash # Check other migration files for removeColumn usage patterns rg -A 5 -B 2 "removeColumn" --type jsLength of output: 37
I want to see how other migrations invoke removeColumn (without restricting to
.js), so let’s search the entire migrations folder:
🏁 Script executed:
#!/bin/bash # Search for removeColumn usage in all migration files rg -A 5 -B 2 "removeColumn" backend/migrationsLength of output: 3845
removeColumn usage is consistent with existing migrations
The third argument you’re passing (the
{ type: DataTypes.TEXT, allowNull: true }object) matches the pattern in other migration files, so the syntax is correct and can be left as-is.That said, please double-check that the column type here matches its original definition to ensure rollbacks work as expected:
• File: backend/migrations/2025052200001-update-sponsorship-table.cjs
• Lines: 5–12
• Verify in the migration that addedIS_PUBLICthat you indeed usedDataTypes.TEXT(and not a boolean type) so yourdown()correctly recreates the column.backend/package.json (1)
3-3: LGTM! Appropriate version bump for minor release.The version bump from "4.0.2" to "4.1.0" correctly follows semantic versioning conventions for a minor release that includes new features (legacy transaction enforcement) and behavioral changes (validation removals).
backend/src/routes/admin-routes.ts (1)
21-33: Excellent refactoring - improved maintainability and type safety.This change from positional arguments to a named parameter object significantly improves code maintainability:
- Better readability: Named parameters make the constructor call self-documenting
- Type safety: Object parameter approach reduces risk of argument order errors
- Future-proof: Adding new parameters won't break existing calls
- Consistency: Aligns with the new
skipType2Txnsconfiguration for legacy transaction handlingThe mapping of all existing parameters is correct and the new
skipType2Txnsparameter properly implements theENFORCE_LEGACY_TRANSACTIONS_CHAINSenvironment variable functionality.backend/src/server.ts (1)
64-76: Consistent implementation of the Paymaster constructor refactoring.This change perfectly mirrors the refactoring pattern applied across the codebase:
- Consistent approach: Identical object parameter structure as used in admin-routes.ts and whitelist-routes.ts
- Correct parameter mapping: All existing configuration values properly preserved
- Feature integration: The
skipType2Txnsparameter correctly integrates the new legacy transaction handling capabilityThe coordinated refactoring across multiple files demonstrates good engineering practices and maintains consistency throughout the application.
backend/src/routes/whitelist-routes.ts (1)
16-28: Consistent completion of the Paymaster constructor refactoring pattern.This change completes the coordinated refactoring across all route files, maintaining perfect consistency:
- Unified approach: Identical object parameter structure used throughout the codebase
- Complete migration: All Paymaster instantiations now use the improved constructor pattern
- Proper configuration: The
skipType2Txnsparameter correctly implements the legacy transaction chain configuration- Maintainable code: Named parameters make future modifications much safer and clearer
The consistent application of this refactoring pattern across admin-routes.ts, server.ts, and whitelist-routes.ts demonstrates excellent coordination and attention to code quality.
backend/src/routes/paymaster-routes.ts (2)
233-233: LGTM: Network validation logic correctly implements the new policy flag.The change properly allows requests to proceed when the sponsorship policy is applicable to all networks (
isApplicableToAllNetworks) even if the specific chain ID is not in the enabled chains list. This aligns with the PR objective.
368-368: LGTM: Consistent network validation logic for VPS mode.This change mirrors the logic from the sponsor mode (line 233) and correctly implements the same network validation behavior for VPS mode.
backend/src/routes/deposit-route.ts (2)
15-27: LGTM: Excellent constructor refactoring with improved maintainability.The refactoring from multiple positional arguments to a single configuration object is a significant improvement:
Benefits:
- Better maintainability: Named parameters make the code self-documenting
- Extensibility: Easy to add new configuration options without breaking existing calls
- Type safety: The interface ensures all required parameters are provided
- Consistency: Aligns with the pattern used across other route files
The integration of
skipType2Txnsfrom the environment variableENFORCE_LEGACY_TRANSACTIONS_CHAINSis correctly implemented.
70-70: LGTM: Minor formatting improvements enhance readability.The formatting changes improve code consistency and readability without affecting functionality.
Also applies to: 106-108, 111-113, 116-118, 124-129
backend/src/paymaster/index.ts (3)
53-65: LGTM: Well-designed interface for constructor parameters.The
ConstructorParamsinterface is excellently structured:
- Type safety: Ensures all required parameters are provided
- Documentation: Self-documenting through named properties
- Extensibility: Easy to add new configuration options
- Consistency: Follows TypeScript best practices
The inclusion of
skipType2Txnsas a string array aligns with how environment variables are typically handled.
84-99: LGTM: Robust constructor implementation with proper data validation.The constructor refactoring is well-implemented:
Strengths:
- Single responsibility: Clean separation of concerns
- Data transformation: Proper conversion from string array to filtered number array
- Error handling:
isNaNfiltering prevents invalid chain IDs- Backwards compatibility: Maintains all existing functionality
The
skipType2Txnsprocessing correctly handles the conversion:this.skipType2Txns = params.skipType2Txns.map( (value) => Number(value) ).filter((value) => !isNaN(value));This ensures only valid numeric chain IDs are included in the skip list.
1135-1135: LGTM: Correct implementation of legacy transaction logic.The transaction fee logic correctly implements the requirement to use legacy transactions for specified chains:
Logic Analysis:
- Condition 1:
!feeData.maxFeePerGas- Handles networks that don't support EIP-1559- Condition 2:
this.skipType2Txns.includes(chainId)- Enforces legacy transactions for configured chains- Fallback: Uses legacy
gasPricewhen either condition is true- Default: Uses EIP-1559 parameters (
maxFeePerGas,maxPriorityFeePerGas,type: 2) otherwiseThis implementation correctly addresses the PR objective to "skip type 2 transactions and send them as legacy transactions instead" for specified chains.
Applied consistently across methods:
whitelistAddressesremoveWhitelistAddressdepositdeployVpaddStakeAlso applies to: 1188-1188, 1252-1252, 1315-1315, 1362-1362
| ## [4.1.0] - 2025-05-22 | ||
| ### Fixes | ||
| - If `isApplicableTOAllChains` is true then dont check `enabledChains` on policy | ||
| - Removed whitelist validation for now | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix markdown formatting issues while maintaining good changelog content.
The changelog entry accurately documents the changes, but there are formatting issues that should be addressed for consistency with markdown standards.
Apply this diff to fix the formatting:
+
## [4.1.0] - 2025-05-22
+
### Fixes
+
- If `isApplicableTOAllChains` is true then dont check `enabledChains` on policy
- Removed whitelist validation for now
+📝 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.
| ## [4.1.0] - 2025-05-22 | |
| ### Fixes | |
| - If `isApplicableTOAllChains` is true then dont check `enabledChains` on policy | |
| - Removed whitelist validation for now | |
| ## [4.1.0] - 2025-05-22 | |
| ### Fixes | |
| - If `isApplicableTOAllChains` is true then dont check `enabledChains` on policy | |
| - Removed whitelist validation for now | |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
4-4: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
🤖 Prompt for AI Agents
In backend/CHANGELOG.md around lines 2 to 6, fix markdown formatting issues by
ensuring proper spacing and heading styles. Add a blank line before and after
the version heading and the "Fixes" subheading, and ensure list items are
properly indented with consistent dashes and spacing. This will improve
readability and maintain consistency with markdown standards.
| /* Removed Whitelist for now | ||
| const isWhitelisted = await checkWhitelist(api_key, epVersion, userOp.sender, sponsorshipPolicy.id); | ||
| // For EPV_06 we still use the old paymaster which whitelists the address on-chain if its verifyingPaymaster it goes to case vps for EPV_06 which checks on db | ||
| if (!isWhitelisted && epVersion !== EPVersions.EPV_06) { | ||
| throw new Error('This sender address has not been whitelisted yet'); | ||
| } | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security concern: Complete removal of whitelist validation.
The whitelist validation has been entirely disabled by commenting out the code. While the PR indicates this is temporary, this creates a significant security gap as sender addresses are no longer validated against whitelists.
Recommendations:
- Implement a feature flag to control whitelist validation instead of commenting out code
- Add monitoring/logging for requests that would have been blocked by whitelist validation
- Set a clear timeline for re-enabling this security feature
- Remove commented code entirely rather than leaving it in the codebase
Consider implementing a feature flag approach:
- /* Removed Whitelist for now
- const isWhitelisted = await checkWhitelist(api_key, epVersion, userOp.sender, sponsorshipPolicy.id);
- // For EPV_06 we still use the old paymaster which whitelists the address on-chain if its verifyingPaymaster it goes to case vps for EPV_06 which checks on db
- if (!isWhitelisted && epVersion !== EPVersions.EPV_06) {
- throw new Error('This sender address has not been whitelisted yet');
- }
- */
+ const whitelistEnabled = process.env.ENABLE_WHITELIST_VALIDATION !== 'false';
+ if (whitelistEnabled) {
+ const isWhitelisted = await checkWhitelist(api_key, epVersion, userOp.sender, sponsorshipPolicy.id);
+ if (!isWhitelisted && epVersion !== EPVersions.EPV_06) {
+ throw new Error('This sender address has not been whitelisted yet');
+ }
+ }Also applies to: 394-399, 599-615
🤖 Prompt for AI Agents
In backend/src/routes/paymaster-routes.ts around lines 258 to 264, the whitelist
validation code is commented out, creating a security risk by disabling sender
address checks. Instead of commenting out, implement a feature flag to toggle
whitelist validation on or off. Add logging to monitor requests that would fail
whitelist checks when disabled. Remove the commented code entirely after
implementing the feature flag and ensure a timeline is set to re-enable the
whitelist validation. Apply the same changes to lines 394-399 and 599-615.
ch4r10t33r
left a comment
ch4r10t33r
left a comment
There was a problem hiding this comment.
LGTM. Please ensure the new configuration variable is enabled.
Description
isApplicableTOAllChainsis true then dont checkenabledChainson policyTypes of changes
What types of changes does your code introduce?
Further comments (optional)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
isPublicproperty from sponsorship policies and related data structures.Chores
IS_PUBLICcolumn from sponsorship policies.