- 
                Notifications
    
You must be signed in to change notification settings  - Fork 0
 
Feature/x402 node #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm learning! Leave a 👍 or a 👎 to help me learn. Extra points if you leave a comment telling me why i'm wrong tagging @greptileai
13 files reviewed, 3 comments
| if (typeof actual[key] !== 'object' || actual[key] === null) { | ||
| missing.push('Invalid type at ' + currentPath + ': expected object'); | ||
| } else { | ||
| checkShape(expected[key], actual[key], currentPath); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: checkShape recursive call doesn't accumulate errors from nested validation
| checkShape(expected[key], actual[key], currentPath); | |
| missing.push(...checkShape(expected[key], actual[key], currentPath)); | 
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/executions/validation/paymentValidation.ts
Line: 50:50
Comment:
**logic:** `checkShape` recursive call doesn't accumulate errors from nested validation
```suggestion
				missing.push(...checkShape(expected[key], actual[key], currentPath));
```
How can I resolve this? If you propose a fix, please make it concise.| import { generateX402Error, generateResponse } from './response/paymentResponse'; | ||
| 
               | 
          ||
| export async function webhookTrigger(this: IWebhookFunctions): Promise<IWebhookResponseData> { | ||
| console.log("AAAAAAAA"); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Remove debug log statement
| console.log("AAAAAAAA"); | 
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/executions/Webhooks.ts
Line: 13:13
Comment:
**style:** Remove debug log statement
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.| type: 'string', | ||
| required: true, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Misleading description - says "EVM address with leading 0x" but this node is for Solana tokens (SOL/USDC), which use base58-encoded addresses, not EVM addresses
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/CrossmintWebhooks.node.ts
Line: 165:166
Comment:
**logic:** Misleading description - says "EVM address with leading 0x" but this node is for Solana tokens (SOL/USDC), which use base58-encoded addresses, not EVM addresses
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm learning! Leave a 👍 or a 👎 to help me learn. Extra points if you leave a comment telling me why i'm wrong tagging @greptileai
2 files reviewed, 1 comment
| @@ -0,0 +1,53 @@ | |||
| export interface IPaymentPayload { | |||
| x402Version: number; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Type mismatch - x402Version changed from string to number but validation expects 'number' type, and the facilitator converts it with String(). Check if incoming x-payment headers actually send numbers or strings.
If headers send strings, this breaks validation at paymentValidation.ts:50. If they send numbers, the String() conversion works but seems unnecessary.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/executions/types/x402Types.ts
Line: 2:2
Comment:
**logic:** Type mismatch - `x402Version` changed from `string` to `number` but validation expects `'number'` type, and the facilitator converts it with `String()`. Check if incoming x-payment headers actually send numbers or strings.
If headers send strings, this breaks validation at `paymentValidation.ts:50`. If they send numbers, the `String()` conversion works but seems unnecessary.
How can I resolve this? If you propose a fix, please make it concise.2176bd8    to
    72f5a20      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm learning! Leave a 👍 or a 👎 to help me learn. Extra points if you leave a comment telling me why i'm wrong tagging @greptileai
7 files reviewed, 5 comments
| const paymentRequirementsObj: IPaymentRequirements = { | ||
| scheme: paymentRequirements.scheme, | ||
| network: paymentRequirements.network, | ||
| maxAmountRequired: paymentRequirements.maxAmountRequired, | ||
| resource: paymentRequirements.resource, | ||
| description: paymentRequirements.description, | ||
| mimeType: paymentRequirements.mimeType, | ||
| outputSchema: paymentRequirements.outputSchema, | ||
| payTo: paymentRequirements.payTo, | ||
| maxTimeoutSeconds: paymentRequirements.maxTimeoutSeconds, | ||
| asset: paymentRequirements.asset, | ||
| extra: paymentRequirements.extra, | ||
| }; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: PR description claims "Trimmed paymentRequirements to only include fields Coinbase expects: scheme, network, payTo, maxAmountRequired, asset" but this code still includes ALL fields (resource, description, mimeType, outputSchema, maxTimeoutSeconds, extra). If Coinbase rejects extra fields, this hasn't been fixed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/executions/facilitator/coinbaseFacilitator.ts
Line: 40:52
Comment:
**logic:** PR description claims "Trimmed `paymentRequirements` to only include fields Coinbase expects: `scheme`, `network`, `payTo`, `maxAmountRequired`, `asset`" but this code still includes ALL fields (`resource`, `description`, `mimeType`, `outputSchema`, `maxTimeoutSeconds`, `extra`). If Coinbase rejects extra fields, this hasn't been fixed.
How can I resolve this? If you propose a fix, please make it concise.| extra: paymentRequirements.extra, | ||
| }; | ||
| const requestBody = { | ||
| x402Version: typeof paymentPayload.x402Version === 'string' ? parseInt(paymentPayload.x402Version, 10) : paymentPayload.x402Version ?? 1, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Duplicate x402Version at both top level AND nested in paymentPayload. PR description says this was fixed ("Removed duplicate top-level x402Version"), but both are still present in the request body.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/executions/facilitator/coinbaseFacilitator.ts
Line: 54:54
Comment:
**logic:** Duplicate `x402Version` at both top level AND nested in `paymentPayload`. PR description says this was fixed ("Removed duplicate top-level `x402Version`"), but both are still present in the request body.
How can I resolve this? If you propose a fix, please make it concise.| x402Version: typeof paymentPayload.x402Version === 'string' ? parseInt(paymentPayload.x402Version, 10) : paymentPayload.x402Version ?? 1, | ||
| paymentPayload: { | ||
| ...paymentPayload, | ||
| x402Version: typeof paymentPayload.x402Version === 'string' ? parseInt(paymentPayload.x402Version, 10) : paymentPayload.x402Version ?? 1, | ||
| }, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Type conversion logic assumes x402Version might not be a string, but IPaymentPayload interface at types/x402Types.ts:3 defines it as string, and validation at validation/paymentValidation.ts:11 expects 'string'. If incoming data is always a string per the interface, this defensive check is unnecessary. If it can be a number, the interface/validation are wrong.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/executions/facilitator/coinbaseFacilitator.ts
Line: 54:58
Comment:
**logic:** Type conversion logic assumes `x402Version` might not be a string, but `IPaymentPayload` interface at `types/x402Types.ts:3` defines it as `string`, and validation at `validation/paymentValidation.ts:11` expects `'string'`. If incoming data is always a string per the interface, this defensive check is unnecessary. If it can be a number, the interface/validation are wrong.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| 
               | 
          ||
| // Map 'base' to the expected network (base or base-sepolia based on environment) | ||
| const normalizedNetwork = network === 'base' ? expectedNetwork : network; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Network normalization only handles 'base' → expectedNetwork mapping. PR description mentions "converting solana-devnet → solana" but no such normalization exists in the code. If Coinbase expects solana instead of solana-devnet, payments will fail.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/executions/helpers/paymentHelpers.ts
Line: 60:60
Comment:
**logic:** Network normalization only handles `'base'` → `expectedNetwork` mapping. PR description mentions "converting `solana-devnet` → `solana`" but no such normalization exists in the code. If Coinbase expects `solana` instead of `solana-devnet`, payments will fail.
How can I resolve this? If you propose a fix, please make it concise.| // Convert payment amount to atomic units (USDC has 6 decimals) | ||
| // If paymentAmount is already in atomic units (integer >= 1000000), use it as-is | ||
| // Otherwise, assume it's in dollars and multiply by 10^6 | ||
| const paymentAmountInAtomicUnits = configuredToken.paymentAmount < 1000000 | ||
| ? Math.floor(configuredToken.paymentAmount * 1000000) | ||
| : configuredToken.paymentAmount; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Heuristic breaks for payments < $1 in atomic units. E.g., 500000 atomic units ($0.50) gets incorrectly multiplied by 10^6 → 500000000000 ($500k). The node description at CrossmintWebhooks.node.ts:178 says "in atomic units (must be an integer)", so this conversion shouldn't exist. Either remove the heuristic or update the UI description to clarify it accepts both formats.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/executions/helpers/paymentHelpers.ts
Line: 78:83
Comment:
**logic:** Heuristic breaks for payments < $1 in atomic units. E.g., 500000 atomic units ($0.50) gets incorrectly multiplied by 10^6 → 500000000000 ($500k). The node description at `CrossmintWebhooks.node.ts:178` says "in atomic units (must be an integer)", so this conversion shouldn't exist. Either remove the heuristic or update the UI description to clarify it accepts both formats.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm learning! Leave a 👍 or a 👎 to help me learn. Extra points if you leave a comment telling me why i'm wrong tagging @greptileai
8 files reviewed, 5 comments
| x402Version: typeof paymentPayload.x402Version === 'string' ? parseInt(paymentPayload.x402Version, 10) : paymentPayload.x402Version ?? 1, | ||
| paymentPayload: { | ||
| ...paymentPayload, | ||
| x402Version: typeof paymentPayload.x402Version === 'string' ? parseInt(paymentPayload.x402Version, 10) : paymentPayload.x402Version ?? 1, | ||
| }, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: PR claims "Removed duplicate top-level x402Version" but both still exist. Line 54 sets top-level x402Version AND line 57 sets it again inside nested paymentPayload. This creates the exact duplicate field issue the PR was supposed to fix.
| x402Version: typeof paymentPayload.x402Version === 'string' ? parseInt(paymentPayload.x402Version, 10) : paymentPayload.x402Version ?? 1, | |
| paymentPayload: { | |
| ...paymentPayload, | |
| x402Version: typeof paymentPayload.x402Version === 'string' ? parseInt(paymentPayload.x402Version, 10) : paymentPayload.x402Version ?? 1, | |
| }, | |
| const requestBody = { | |
| paymentPayload: { | |
| ...paymentPayload, | |
| x402Version: typeof paymentPayload.x402Version === 'string' ? parseInt(paymentPayload.x402Version, 10) : paymentPayload.x402Version ?? 1, | |
| }, | |
| paymentRequirements: paymentRequirementsObj, | |
| }; | 
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/executions/facilitator/coinbaseFacilitator.ts
Line: 54:58
Comment:
**logic:** PR claims "Removed duplicate top-level `x402Version`" but both still exist. Line 54 sets top-level `x402Version` AND line 57 sets it again inside nested `paymentPayload`. This creates the exact duplicate field issue the PR was supposed to fix.
```suggestion
	const requestBody = {
		paymentPayload: {
			...paymentPayload,
			x402Version: typeof paymentPayload.x402Version === 'string' ? parseInt(paymentPayload.x402Version, 10) : paymentPayload.x402Version ?? 1,
		},
		paymentRequirements: paymentRequirementsObj,
	};
```
How can I resolve this? If you propose a fix, please make it concise.| const paymentRequirementsObj: IPaymentRequirements = { | ||
| scheme: paymentRequirements.scheme, | ||
| network: paymentRequirements.network, | ||
| maxAmountRequired: paymentRequirements.maxAmountRequired, | ||
| resource: paymentRequirements.resource, | ||
| description: paymentRequirements.description, | ||
| mimeType: paymentRequirements.mimeType, | ||
| outputSchema: paymentRequirements.outputSchema, | ||
| payTo: paymentRequirements.payTo, | ||
| maxTimeoutSeconds: paymentRequirements.maxTimeoutSeconds, | ||
| asset: paymentRequirements.asset, | ||
| extra: paymentRequirements.extra, | ||
| }; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: PR claims "Trimmed paymentRequirements to only include fields Coinbase expects: scheme, network, payTo, maxAmountRequired, asset" but ALL original fields remain (resource, description, mimeType, outputSchema, maxTimeoutSeconds, extra). The supposed trimming never happened - these extra fields will still cause Coinbase API rejection.
| const paymentRequirementsObj: IPaymentRequirements = { | |
| scheme: paymentRequirements.scheme, | |
| network: paymentRequirements.network, | |
| maxAmountRequired: paymentRequirements.maxAmountRequired, | |
| resource: paymentRequirements.resource, | |
| description: paymentRequirements.description, | |
| mimeType: paymentRequirements.mimeType, | |
| outputSchema: paymentRequirements.outputSchema, | |
| payTo: paymentRequirements.payTo, | |
| maxTimeoutSeconds: paymentRequirements.maxTimeoutSeconds, | |
| asset: paymentRequirements.asset, | |
| extra: paymentRequirements.extra, | |
| }; | |
| const paymentRequirementsObj: IPaymentRequirements = { | |
| scheme: paymentRequirements.scheme, | |
| network: paymentRequirements.network, | |
| payTo: paymentRequirements.payTo, | |
| maxAmountRequired: paymentRequirements.maxAmountRequired, | |
| asset: paymentRequirements.asset, | |
| }; | 
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/executions/facilitator/coinbaseFacilitator.ts
Line: 40:52
Comment:
**logic:** PR claims "Trimmed `paymentRequirements` to only include fields Coinbase expects: `scheme`, `network`, `payTo`, `maxAmountRequired`, `asset`" but ALL original fields remain (`resource`, `description`, `mimeType`, `outputSchema`, `maxTimeoutSeconds`, `extra`). The supposed trimming never happened - these extra fields will still cause Coinbase API rejection.
```suggestion
	const paymentRequirementsObj: IPaymentRequirements = {
		scheme: paymentRequirements.scheme,
		network: paymentRequirements.network,
		payTo: paymentRequirements.payTo,
		maxAmountRequired: paymentRequirements.maxAmountRequired,
		asset: paymentRequirements.asset,
	};
```
How can I resolve this? If you propose a fix, please make it concise.| @@ -0,0 +1,54 @@ | |||
| // EVM exact scheme payload structure (EIP-3009) | |||
| export interface IPaymentPayload { | |||
| x402Version: string; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Type mismatch breaks validation. Interface defines x402Version: string but paymentValidation.ts:11 expects 'number' type, causing validation to fail with "Invalid type at x402Version: expected number, got string". Either change this to number or fix validation to expect 'string'.
| x402Version: string; | |
| x402Version: number; | 
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/executions/types/x402Types.ts
Line: 3:3
Comment:
**logic:** Type mismatch breaks validation. Interface defines `x402Version: string` but `paymentValidation.ts:11` expects `'number'` type, causing validation to fail with "Invalid type at x402Version: expected number, got string". Either change this to `number` or fix validation to expect `'string'`.
```suggestion
	x402Version: number;
```
How can I resolve this? If you propose a fix, please make it concise.| return null; | ||
| } | ||
| 
               | 
          ||
| // Map 'base' to the expected network (base or base-sepolia based on environment) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: PR claims "Added network normalization to convert solana-devnet → solana" but only 'base' → expectedNetwork mapping exists. No solana-devnet normalization implemented - Coinbase API calls will fail if it expects solana instead of solana-devnet.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/executions/helpers/paymentHelpers.ts
Line: 59:59
Comment:
**logic:** PR claims "Added network normalization to convert `solana-devnet` → `solana`" but only `'base'` → `expectedNetwork` mapping exists. No solana-devnet normalization implemented - Coinbase API calls will fail if it expects `solana` instead of `solana-devnet`.
How can I resolve this? If you propose a fix, please make it concise.| // Convert payment amount to atomic units (USDC has 6 decimals) | ||
| // If paymentAmount is already in atomic units (integer >= 1000000), use it as-is | ||
| // Otherwise, assume it's in dollars and multiply by 10^6 | ||
| const paymentAmountInAtomicUnits = configuredToken.paymentAmount < 1000000 | ||
| ? Math.floor(configuredToken.paymentAmount * 1000000) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Heuristic incorrectly converts sub-dollar amounts. E.g., 500000 atomic units (0.50 USDC) incorrectly becomes 500000000000 ($500k USDC) because 500000 < 1000000 triggers multiplication. Either remove heuristic entirely (since CrossmintWebhooks.node.ts:178 says "in atomic units") or use threshold like < 100 to detect dollar amounts.
| // Convert payment amount to atomic units (USDC has 6 decimals) | |
| // If paymentAmount is already in atomic units (integer >= 1000000), use it as-is | |
| // Otherwise, assume it's in dollars and multiply by 10^6 | |
| const paymentAmountInAtomicUnits = configuredToken.paymentAmount < 1000000 | |
| ? Math.floor(configuredToken.paymentAmount * 1000000) | |
| // Only convert if value is clearly in dollars (< 100) vs atomic units (>= 100) | |
| const paymentAmountInAtomicUnits = configuredToken.paymentAmount < 100 | |
| ? Math.floor(configuredToken.paymentAmount * 1000000) | |
| : configuredToken.paymentAmount; | 
Prompt To Fix With AI
This is a comment left during a code review.
Path: nodes/webhooks/executions/helpers/paymentHelpers.ts
Line: 78:82
Comment:
**logic:** Heuristic incorrectly converts sub-dollar amounts. E.g., 500000 atomic units (0.50 USDC) incorrectly becomes 500000000000 ($500k USDC) because 500000 < 1000000 triggers multiplication. Either remove heuristic entirely (since `CrossmintWebhooks.node.ts:178` says "in atomic units") or use threshold like `< 100` to detect dollar amounts.
```suggestion
		// Only convert if value is clearly in dollars (< 100) vs atomic units (>= 100)
		const paymentAmountInAtomicUnits = configuredToken.paymentAmount < 100
			? Math.floor(configuredToken.paymentAmount * 1000000)
			: configuredToken.paymentAmount;
```
How can I resolve this? If you propose a fix, please make it concise.
Fix Coinbase CDP API schema validation errors in x402 payment verification
Summary
Fixed a 400 Bad Request error from Coinbase CDP API that was occurring during x402 payment verification. The error was:
"request body has an error: doesn't match schema: value doesn't match any schema from oneOf".The root causes were:
x402Versionfield (both top-level and nested inpaymentPayload)paymentRequirementsthat Coinbase doesn't acceptsolana-devnetvssolana)x402Versionbetween interface and validationChanges made:
x402Versionfrom request body sent to CoinbasepaymentRequirementsto only include fields Coinbase expects:scheme,network,payTo,maxAmountRequired,assetsolana-devnet→solanafor Coinbase API compatibilityx402Versiontype fromstringtonumberinIPaymentPayloadinterface to match validation logicReview & Testing Checklist for Human
solana-devnet→solanais correct for Coinbase API. Check if this breaks devnet testing or if Coinbase expects the original network value.paymentRequirementsfields match their/platform/v2/x402/verifyand/platform/v2/x402/settleschemas.x402Versionfromstringtonumberin theIPaymentPayloadinterface.Test Plan
Notes