[SDK] Make maxAmount optional in wrapFetchWithPayment #8369
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: a87db28 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
WalkthroughMakes the maxValue parameter optional in wrapFetchWithPayment and removes runtime schema validation from decodePayment; payment amount checks now run only when maxValue is provided. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant wrapFetch as wrapFetchWithPayment
participant decode as decodePayment
participant PaymentCheck as MaxAmountCheck
participant Fetch as fetch
Client->>wrapFetch: initiate request (maybe maxValue)
wrapFetch->>decode: decode payment payload
note right of decode `#D3F9D8`: decodePayment no longer validates against schema
decode-->>wrapFetch: parsed payment
alt maxValue provided
wrapFetch->>PaymentCheck: compare selectedPaymentRequirements.maxAmountRequired <= maxValue
PaymentCheck-->>wrapFetch: allowed / error
else maxValue undefined
note right of wrapFetch `#F3F4F6`: skip max-amount check
end
wrapFetch->>Fetch: perform fetch with selected payment
Fetch-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
packages/thirdweb/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-08-28T20:50:33.170ZApplied to files:
🧬 Code graph analysis (1)packages/thirdweb/src/x402/schemas.ts (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). (8)
🔇 Additional comments (2)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/thirdweb/src/x402/encode.ts (1)
36-44: Update TSDoc to reflect the absence of validation.The TSDoc on line 34 states "The decoded and validated PaymentPayload object" but validation has been removed. The function now only decodes without validating the structure.
Apply this diff to update the documentation:
/** * Decodes a base64 encoded payment string back into a PaymentPayload object * * @param payment - The base64 encoded payment string to decode - * @returns The decoded and validated PaymentPayload object + * @returns The decoded PaymentPayload object (not validated at runtime) */packages/thirdweb/src/x402/fetchWithPayment.ts (1)
26-26: Update TSDoc to reflect the optional maxValue parameter.The documentation states "defaults to 1 USDC" (line 26) and unconditionally mentions throwing an error if payment exceeds maximum (line 45-46), but
maxValueis now optional and the check only runs when provided.Apply this diff to update the documentation:
* @param fetch - The fetch function to wrap (typically globalThis.fetch) * @param client - The thirdweb client used to access RPC infrastructure * @param wallet - The wallet used to sign payment messages - * @param maxValue - The maximum allowed payment amount in base units (defaults to 1 USDC) + * @param maxValue - Optional maximum allowed payment amount in base units. If not provided, no amount limit is enforced. * @returns A wrapped fetch function that handles 402 responses automatically* - * @throws {Error} If the payment amount exceeds the maximum allowed value + * @throws {Error} If maxValue is provided and the payment amount exceeds it * @throws {Error} If a payment has already been attempted for this requestAlso applies to: 45-46
🧹 Nitpick comments (1)
.changeset/famous-owls-lick.md (1)
1-5: Minor terminology inconsistency in changeset description.The description mentions "Make maxAmount optional" but the actual parameter name in
wrapFetchWithPaymentismaxValue(notmaxAmount).Consider updating for consistency:
--- "thirdweb": patch --- -Make maxAmount optional in wrapFetchWithPayment and loosen schema validation for payment payloads +Make maxValue optional in wrapFetchWithPayment and loosen schema validation for payment payloads
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/famous-owls-lick.md(1 hunks)packages/thirdweb/src/x402/encode.ts(2 hunks)packages/thirdweb/src/x402/fetchWithPayment.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
**/*.{ts,tsx}: Use explicit function declarations and explicit return types in TypeScript
Limit each file to one stateless, single‑responsibility function
Re‑use shared types from@/typeswhere applicable
Prefertypealiases overinterfaceexcept for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Prefer composition over inheritance; use utility types (Partial, Pick, etc.)
Lazy‑import optional features and avoid top‑level side‑effects to reduce bundle size
Files:
packages/thirdweb/src/x402/encode.tspackages/thirdweb/src/x402/fetchWithPayment.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/x402/encode.tspackages/thirdweb/src/x402/fetchWithPayment.ts
packages/thirdweb/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/thirdweb/**/*.{ts,tsx}: Every public symbol must have comprehensive TSDoc with at least one compiling@exampleand a custom tag (@beta,@internal,@experimental, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Lazy‑load heavy dependencies inside async paths (e.g.,const { jsPDF } = await import("jspdf"))
Files:
packages/thirdweb/src/x402/encode.tspackages/thirdweb/src/x402/fetchWithPayment.ts
.changeset/*.md
📄 CodeRabbit inference engine (AGENTS.md)
.changeset/*.md: Each change inpackages/*must include a changeset for the appropriate package
Version bump rules: patch for non‑API changes; minor for new/modified public API
Files:
.changeset/famous-owls-lick.md
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
📚 Learning: 2025-08-29T15:37:38.513Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-08-29T15:37:38.513Z
Learning: Applies to .changeset/*.md : Version bump rules: patch for non‑API changes; minor for new/modified public API
Applied to files:
.changeset/famous-owls-lick.md
📚 Learning: 2025-08-29T15:37:38.513Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-08-29T15:37:38.513Z
Learning: Surface breaking changes prominently in PR descriptions
Applied to files:
.changeset/famous-owls-lick.md
📚 Learning: 2025-06-26T19:46:04.024Z
Learnt from: gregfromstl
Repo: thirdweb-dev/js PR: 7450
File: packages/thirdweb/src/bridge/Webhook.ts:57-81
Timestamp: 2025-06-26T19:46:04.024Z
Learning: In the onramp webhook schema (`packages/thirdweb/src/bridge/Webhook.ts`), the `currencyAmount` field is intentionally typed as `z.number()` while other amount fields use `z.string()` because `currencyAmount` represents fiat currency amounts in decimals (like $10.50), whereas other amount fields represent token amounts in wei (very large integers that benefit from bigint representation). The different naming convention (`currencyAmount` vs `amount`) reflects this intentional distinction.
Applied to files:
packages/thirdweb/src/x402/fetchWithPayment.ts
📚 Learning: 2025-07-18T19:19:55.613Z
Learnt from: CR
Repo: thirdweb-dev/js PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Applied to files:
packages/thirdweb/src/x402/fetchWithPayment.ts
⏰ 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). (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Size
- GitHub Check: Build Packages
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/thirdweb/src/x402/fetchWithPayment.ts (1)
55-55: Verify the version bump for this public API change.Making
maxValueoptional changes the public function signature ofwrapFetchWithPayment. The conditional check correctly guards the validation.Per coding guidelines, modifying a public API typically requires a minor version bump rather than a patch. Since this changes the function signature (even though it's backward-compatible), please verify that a patch release is appropriate for this change, or update the changeset to indicate a minor bump.
As per coding guidelines.
Also applies to: 94-101
size-limit report 📦
|
joaquim-verges
force-pushed
the
_SDK_Make_maxAmount_optional_in_wrapFetchWithPayment
branch
from
8d30aeb to
a87db28
Compare
joaquim-verges
deleted the
_SDK_Make_maxAmount_optional_in_wrapFetchWithPayment
branch
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8369 +/- ##
==========================================
- Coverage 54.64% 54.63% -0.01%
==========================================
Files 919 919
Lines 60693 60693
Branches 4111 4113 +2
==========================================
- Hits 33167 33162 -5
- Misses 27424 27429 +5
Partials 102 102
🚀 New features to boost your workflow:
|
PR-Codex overview
This PR focuses on making the
maxAmountparameter optional in thewrapFetchWithPaymentfunction and relaxing the validation schema for payment payloads.Detailed summary
maxAmountoptional inwrapFetchWithPayment.RequestedPaymentPayloadSchemato remove thenetworkproperty.decodePaymentfunction by removing schema validation.Summary by CodeRabbit