PRO-3097-MTP_V07 #201
PRO-3097-MTP_V07 #201
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces support for a new version (V07) of the MultiTokenPaymaster contract throughout the backend. It adds new columns to the relevant database tables and updates the Sequelize models accordingly. The Paymaster class is extended with new properties and methods to handle V07-specific signing and gas limit logic. Configuration files and route handlers are updated to accommodate the new paymaster version and its associated gas limit parameters. The API and repository layers are modified to support querying and returning data for multiple entry point versions, ensuring compatibility with both EPV_06 and EPV_07. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API Server
participant Paymaster Class
participant DB
participant MultiTokenPaymaster Contract (V07)
participant Wallet
Client->>API Server: Request paymaster data (EPV_07)
API Server->>DB: Query MultiTokenPaymaster by chainId, token, epVersion
DB-->>API Server: Return paymaster record (with epVersion)
API Server->>Paymaster Class: signMultiTokenPaymasterV07(userOp, ...params)
Paymaster Class->>MultiTokenPaymaster Contract (V07): getHash(userOp, ...params)
Paymaster Class->>Wallet: Sign hash
Paymaster Class->>API Server: Return paymasterData, gas limits
API Server-->>Client: Respond with paymasterData, gas info
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
vignesha22
requested review from
ch4r10t33r,
cryptonoyaiba,
kanthgithub and
nikhilkumar1612
Deploying arka with
|
| Latest commit: |
c5d185d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e361a527.arka-3qg.pages.dev |
| Branch Preview URL: | https://pro-3097-arka-mtp-v07.arka-3qg.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
backend/migrations/20250416000001-update-apiKey-table.cjs(1 hunks)backend/migrations/20250416000002-update-MTP-table.cjs(1 hunks)backend/src/abi/MultiTokenPaymasterAbiV2.ts(1 hunks)backend/src/models/api-key.ts(2 hunks)backend/src/models/multiTokenPaymaster.ts(2 hunks)backend/src/paymaster/index.ts(6 hunks)backend/src/plugins/config.ts(3 hunks)backend/src/repository/multi-token-paymaster.ts(2 hunks)backend/src/routes/admin-routes.ts(1 hunks)backend/src/routes/deposit-route.ts(1 hunks)backend/src/routes/paymaster-routes.ts(10 hunks)backend/src/routes/pimlico-routes.ts(1 hunks)backend/src/routes/whitelist-routes.ts(1 hunks)backend/src/server.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
backend/src/server.ts (1)
backend/src/paymaster/index.ts (1)
Paymaster(51-1340)
backend/src/routes/whitelist-routes.ts (1)
backend/src/paymaster/index.ts (1)
Paymaster(51-1340)
backend/src/routes/admin-routes.ts (1)
backend/src/paymaster/index.ts (1)
Paymaster(51-1340)
backend/src/routes/deposit-route.ts (1)
backend/src/paymaster/index.ts (1)
Paymaster(51-1340)
🪛 Biome (1.9.4)
backend/src/routes/paymaster-routes.ts
[error] 300-300: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
backend/src/paymaster/index.ts
[error] 274-274: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 870-870: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 1328-1328: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (32)
backend/src/plugins/config.ts (3)
41-42: Gas limit configuration added for MultiTokenPaymaster V07These new configuration parameters are properly integrated into the config schema with appropriate defaults and support for environment variable overrides. These gas limit settings will be used by the Paymaster class for V07 transaction processing.
85-86: Default gas limit values look appropriateThe default values (50000 for verification gas limit and 70000 for post-op gas limit) are set with appropriate fallbacks from environment variables.
125-126: Config values properly exposed to the applicationThe configuration values are correctly added to the final config object that gets exposed to the application.
backend/migrations/20250416000001-update-apiKey-table.cjs (1)
4-5: Migration properly adds MULTI_TOKEN_PAYMASTERS_V2 columnThe migration adds the required column with appropriate nullability, which aligns with the V07 paymaster support.
backend/src/routes/whitelist-routes.ts (1)
15-16: Paymaster constructor updated with new gas limit parametersThe Paymaster constructor is correctly updated to include the new MultiTokenPaymaster gas limit parameters (MTP_PVGL and MTP_PPGL) from the configuration, which are required for V07 paymaster support.
backend/src/routes/pimlico-routes.ts (1)
129-131: Added epVersion to paymaster responseThe response now includes the entry point version (epVersion) for each paymaster, which is essential for clients to distinguish between V06 and V07 paymasters.
backend/src/routes/deposit-route.ts (1)
14-15: Updated Paymaster constructor with new gas limit parameters.The constructor now includes the new MTP_PVGL and MTP_PPGL parameters for MultiTokenPaymaster V07 gas limit configuration. This change is required to properly support the V07 version features.
backend/src/server.ts (1)
64-65: Updated Paymaster constructor with MultiTokenPaymaster V07 gas limit parameters.The server initialization now correctly passes the new gas limit parameters (MTP_PVGL and MTP_PPGL) to the Paymaster constructor, which is essential for supporting the V07 functionality across the application.
backend/src/routes/admin-routes.ts (1)
21-22: Updated Paymaster constructor with MultiTokenPaymaster V07 parameters.The constructor now includes the MTP_PVGL and MTP_PPGL parameters needed for V07 support, maintaining consistency with other route files in the application.
backend/src/models/multiTokenPaymaster.ts (2)
10-10: Added epVersion field to track entry point version support.This new required field enables the model to identify which entry point version (EPV_06 or EPV_07) is associated with each MultiTokenPaymaster record, which is essential for the multi-version support being implemented.
54-58: Added Sequelize definition for the epVersion field.The field is correctly defined as a non-nullable string, which aligns with its intended purpose of tracking the entry point version (EPV_06 or EPV_07) for each record.
backend/src/models/api-key.ts (2)
13-13: Property addition looks good.
No issues with declaringmultiTokenPaymastersV2in the model. It follows the existing pattern for optional string columns.
81-85: Database column definition is consistent.
This aligns with the naming and nullable setup of other columns. No concerns here.backend/migrations/20250416000002-update-MTP-table.cjs (1)
8-10: Migration setup is valid.
Thedownfunction properly reverts the schema changes, and exportingup/downis correct.Also applies to: 12-12
backend/src/repository/multi-token-paymaster.ts (2)
3-3: New import is appropriate.
ImportingEPVersionsaligns with the extended filtering logic.
17-21: Method extension is consistent.
Filtering bychainId,tokenAddress, andepVersionis clear and descriptive.backend/src/routes/paymaster-routes.ts (11)
19-19: Destructuring is straightforward.
Extractingpaymasterfromoptionsis a clean approach.
105-105: Variable addition for multiTokenPaymastersV2 is clear.
No issues—this mirrors the approach used for the V1 array.
145-148: Consistent base64 decoding logic.
ReadingmultiTokenPaymastersV2from the API key entity matches the existing pattern.
205-205: Mode override logic.
Switching fromsponsortovpswhenuseVpis passed makes sense. Be sure this behavior is desired to avoid surprises.
325-329: Version-based paymaster logic is clear.
Delegating tosignMultiTokenPaymastervs.signMultiTokenPaymasterV07is well-structured.
390-396: VerifyingPaymasters check is correct.
Returning an error if no entry is found for EPV_06 is consistent with your existing validations.
400-408: VerifyingPaymastersV2 flow is consistent.
Likewise, returning an error if not configured for EPV_07 ensures clarity for users.
412-414: Common ERC20 route extended for epVersion.
CheckingEPV_06vs.EPV_07before calling the repository is logically consistent.
419-420: Private key presence check.
Promptly returning an error is a good security practice.
441-444: Version-based branching looks good.
No issues in separating EPV_06 vs. EPV_07 logic for the paymaster signing calls.
486-489: Cache data retrieval.
Logging and returning success or failure is straightforward. No issues here.backend/src/paymaster/index.ts (4)
24-24: No issues found with the new ABI import.
The import statement forMultiTokenPaymasterAbiV2is straightforward and aligns with the updated V07 paymaster logic.
58-59: Validate usage of string vs. BigNumber for gas limit properties.
These new class properties (MTP_PVGL,MTP_PPGL) are typed as strings. In other places, paymaster gas limits (e.g.,EP7_PVGL) are stored asBigNumber. Double-check consistency to avoid accidental type mismatches or conversions.Would you like to confirm whether these properties should remain strings or be converted to
BigNumberupon assignment for uniformity?
246-275:Details
❓ Verification inconclusive
Include a default price source parameter and fix string concatenation.
- Price source check: You’re calling
getHashwith a hardcoded0for thepriceSourceargument. Confirm ifExchangeRateSource=0is indeed the intended default. Otherwise, consider adding a parameter to make it explicit.- Use a template literal: On line 274, replace the string concatenation with a template literal to comply with the lint hint and improve readability.
- throw new Error('Failed ' + err.message) + throw new Error(`Failed ${err.message}`)Verify that the contract truly expects
priceSource=0in this scenario to avoid incorrect hashing at the contract level.
Action Required: Verify Default Price Source & Update Error String
- Ensure that using a hardcoded
0for thepriceSourceargument in the call togetHashis correct (i.e. thatExchangeRateSource=0is indeed the intended default as per the contract specification). If not, add a parameter to clearly specify the price source.- Replace the string concatenation for the error message with a template literal for improved readability.
- throw new Error('Failed ' + err.message) + throw new Error(`Failed ${err.message}`)🧰 Tools
🪛 Biome (1.9.4)
[error] 274-274: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
779-872:Details
❓ Verification inconclusive
Enhance error readability and finalize V07 logic checks.
- Use a template literal for error messages: On line 870, replace the string concatenation with a template literal to align with lint best practices.
- Logic Confirmation: The method updates
userOptwice (before and after estimating gas) to incorporate the new paymaster data. This seems intentional but confirm no intermittent side effects (e.g., using partial paymaster data on the first pass).- throw new Error('Failed to process request to bundler. Please contact support team RawErrorMsg:' + err.message) + throw new Error(`Failed to process request to bundler. Please contact support team RawErrorMsg: ${err.message}`)Double-check whether staggering the assignment of
userOp.paymasterAndDataacross two estimates is the intended design.
Action: Update Error Message Template and Confirm Double Assignment of Paymaster Data
- Update the error message (line 870) to use a template literal as shown below:
- throw new Error('Failed to process request to bundler. Please contact support team RawErrorMsg:' + err.message) + throw new Error(`Failed to process request to bundler. Please contact support team RawErrorMsg: ${err.message}`)- Verify that the intentional double assignment of
userOp.paymasterAndData(once before and once after the gas estimation) does not introduce any unintended side effects. Confirm that using intermediate paymaster data in the first pass is the desired design.🧰 Tools
🪛 Biome (1.9.4)
[error] 870-870: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
backend/src/abi/MultiTokenPaymasterAbiV2.ts (1)
1-878: ABI file addition looks good.
This ABI definition for MultiTokenPaymaster V2 is comprehensive and consistent with the newly integrated paymaster logic inindex.ts. No issues found.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
backend/CHANGELOG.md(1 hunks)backend/package.json(1 hunks)
🧰 Additional context used
🪛 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 (2)
backend/package.json (1)
3-3: Version bump is appropriate.The version update to 3.1.7 correctly reflects the addition of a new feature. No issues found.
backend/CHANGELOG.md (1)
2-5: Changelog entry is clear and accurate.The changelog entry for version 3.1.7 accurately documents the new feature and aligns with the code changes.
🧰 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)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/migrations/20250416000002-update-MTP-table.cjs (1)
3-5: Consider indexing the new column.Adding an index on
EP_VERSIONmay help if you expect frequent lookups by version. Otherwise, the migration looks fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
backend/migrations/20250416000001-update-apiKey-table.cjs(1 hunks)backend/migrations/20250416000002-update-MTP-table.cjs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
backend/migrations/20250416000001-update-apiKey-table.cjs (1)
1-11: Migration looks good.This migration correctly adds a nullable
MULTI_TOKEN_PAYMASTERS_V2column to the api_keys table with appropriateIF EXISTSclauses in both up and down migrations.
| async function down({ context: queryInterface }) { | ||
| await queryInterface.sequelize.query(`ALTER TABLE "${process.env.DATABASE_SCHEMA_NAME}".multi_token_paymaster DROP COLUMN EP_VERSION;`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add IF EXISTS clause to the down migration.
The down migration should include an IF EXISTS clause to match the style in the up migration and prevent errors if the table doesn't exist.
- await queryInterface.sequelize.query(`ALTER TABLE "${process.env.DATABASE_SCHEMA_NAME}".multi_token_paymaster DROP COLUMN EP_VERSION;`);
+ await queryInterface.sequelize.query(`ALTER TABLE IF EXISTS "${process.env.DATABASE_SCHEMA_NAME}".multi_token_paymaster DROP COLUMN EP_VERSION;`);📝 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.
| async function down({ context: queryInterface }) { | |
| await queryInterface.sequelize.query(`ALTER TABLE "${process.env.DATABASE_SCHEMA_NAME}".multi_token_paymaster DROP COLUMN EP_VERSION;`); | |
| } | |
| async function down({ context: queryInterface }) { | |
| await queryInterface.sequelize.query(`ALTER TABLE IF EXISTS "${process.env.DATABASE_SCHEMA_NAME}".multi_token_paymaster DROP COLUMN EP_VERSION;`); | |
| } |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
backend/src/paymaster/index.ts (1)
1328-1328: 🧹 Nitpick (assertive)Prefer template literal over string concatenation.
ReplacetokenAddress + ' ' + ErrorMessage.COINGECKO_PRICE_NOT_FETCHEDwith a template literal.- throw new Error(tokenAddress + ' ' + ErrorMessage.COINGECKO_PRICE_NOT_FETCHED); + throw new Error(`${tokenAddress} ${ErrorMessage.COINGECKO_PRICE_NOT_FETCHED}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
backend/src/paymaster/index.ts(7 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/paymaster/index.ts
[error] 274-274: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 870-870: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
backend/src/paymaster/index.ts (1)
24-24: Import for MultiTokenPaymasterAbiV2 looks good.
No issues found here.
| this.MTP_PVGL = mtpPvgl; | ||
| this.MTP_PPGL = mtpPpgl; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Keep type consistency for MTP_PVGL and MTP_PPGL.
If you convert MTP_VGL_MARKUP to BigNumber in the constructor, it’s also more consistent to store MTP_PVGL and MTP_PPGL as BigNumber for clarity.
| const response = await provider.send('eth_estimateUserOperationGas', [userOp, entryPoint]); | ||
| if (BigNumber.from(userOp.verificationGasLimit).lt("45000")) userOp.verificationGasLimit = BigNumber.from("45000").toHexString(); // This is to counter the unaccounted cost(45000) | ||
| userOp.verificationGasLimit = BigNumber.from(response.verificationGasLimit).add("30000").toHexString(); // This is added just in case the token is proxy | ||
| userOp.verificationGasLimit = BigNumber.from(response.verificationGasLimit).add(this.MTP_VGL_MARKUP).toHexString(); // This is added just in case the token is proxy |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify numeric input for MTP_VGL_MARKUP.
Ensuring MTP_VGL_MARKUP is a valid BigNumber-compatible string is crucial before calling .add(...). Otherwise, it might lead to runtime errors.
🏁 Script executed:
#!/bin/bash
# Description: Verify that MTP_VGL_MARKUP is always a numeric string
rg --multiline --json "MTP_VGL_MARKUP" Length of output: 5915
Ensure MTP_VGL_MARKUP is Validated as a Numeric Value
The current implementation in backend/src/paymaster/index.ts (line 758) relies on this.MTP_VGL_MARKUP being a valid BigNumber-compatible numeric string. Although the default configuration (in backend/src/plugins/config.ts and backend/demo.env) sets this value to "30000", the fact that it’s sourced from environment variables means there's a risk of receiving non-numeric input. This could lead to runtime errors when calling BigNumber.from(...).add(...).
- Actionable Suggestion:
Add explicit validation or coercion to ensure thatMTP_VGL_MARKUPis numeric before it is used in BigNumber arithmetic. This could involve sanitizing the environment variable upon loading the configuration or implementing a helper function to validate/convert the input.
Description
Types of changes
What types of changes does your code introduce?
Further comments (optional)
Summary by CodeRabbit
New Features
MTP_PVGLandMTP_PPGL).Improvements
multiTokenPaymastersV2andepVersion.Bug Fixes