-
Notifications
You must be signed in to change notification settings - Fork 619
[SDK] Add EIP-5792 support to Coinbase and refactor wallet capabilities #7732
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "thirdweb": patch | ||
| --- | ||
|
|
||
| Improve EIP5792 support | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,8 +14,16 @@ | |
| stringToHex, | ||
| uint8ArrayToHex, | ||
| } from "../../utils/encoding/hex.js"; | ||
| import { stringify } from "../../utils/json.js"; | ||
| import { parseTypedData } from "../../utils/signatures/helpers/parse-typed-data.js"; | ||
| import { COINBASE } from "../constants.js"; | ||
| import { toGetCallsStatusResponse } from "../eip5792/get-calls-status.js"; | ||
| import { toGetCapabilitiesResult } from "../eip5792/get-capabilities.js"; | ||
| import { toProviderCallParams } from "../eip5792/send-calls.js"; | ||
| import type { | ||
| GetCallsStatusRawResponse, | ||
| WalletCapabilities, | ||
| } from "../eip5792/types.js"; | ||
| import type { | ||
| Account, | ||
| SendTransactionOption, | ||
|
|
@@ -269,6 +277,64 @@ | |
| } | ||
| return res; | ||
| }, | ||
| sendCalls: async (options) => { | ||
| try { | ||
| const { callParams, chain } = await toProviderCallParams( | ||
| options, | ||
| account, | ||
| ); | ||
| const callId = await provider.request({ | ||
| method: "wallet_sendCalls", | ||
| params: callParams, | ||
| }); | ||
| if (callId && typeof callId === "object" && "id" in callId) { | ||
| return { chain, client, id: callId.id as string }; | ||
| } | ||
| return { chain, client, id: callId as string }; | ||
| } catch (error) { | ||
| if (/unsupport|not support/i.test((error as Error).message)) { | ||
| throw new Error( | ||
| `${COINBASE} errored calling wallet_sendCalls, with error: ${error instanceof Error ? error.message : stringify(error)}`, | ||
| ); | ||
| } | ||
| throw error; | ||
| } | ||
| }, | ||
| async getCallsStatus(options) { | ||
| try { | ||
| const rawResponse = (await provider.request({ | ||
| method: "wallet_getCallsStatus", | ||
| params: [options.id], | ||
| })) as GetCallsStatusRawResponse; | ||
| return toGetCallsStatusResponse(rawResponse); | ||
| } catch (error) { | ||
| if (/unsupport|not support/i.test((error as Error).message)) { | ||
| throw new Error( | ||
| `${COINBASE} does not support wallet_getCallsStatus, reach out to them directly to request EIP-5792 support.`, | ||
| ); | ||
| } | ||
| throw error; | ||
| } | ||
| }, | ||
| async getCapabilities(options) { | ||
| const chainId = options.chainId; | ||
| try { | ||
| const result = (await provider.request({ | ||
| method: "wallet_getCapabilities", | ||
| params: [getAddress(account.address)], | ||
| })) as Record<string, WalletCapabilities>; | ||
| return toGetCapabilitiesResult(result, chainId); | ||
| } catch (error: unknown) { | ||
| if ( | ||
| /unsupport|not support|not available/i.test((error as Error).message) | ||
| ) { | ||
| return { | ||
| message: `${COINBASE} does not support wallet_getCapabilities, reach out to them directly to request EIP-5792 support.`, | ||
| }; | ||
| } | ||
| throw error; | ||
| } | ||
| }, | ||
|
Comment on lines
+280
to
+337
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainAdd test coverage for Coinbase EIP-5792 methods Static analysis indicates these critical EIP-5792 methods lack test coverage. Would you like me to create test cases for the Coinbase wallet's EIP-5792 implementation? 🏁 Script executed: #!/bin/bash
# Check if there are any tests for Coinbase wallet EIP-5792 functionality
fd -e test.ts -e test.tsx . | xargs rg -l "coinbase.*sendCalls|coinbase.*getCallsStatus|coinbase.*getCapabilities" -iLength of output: 119 🏁 Script executed: #!/bin/bash
# Further verify test coverage for Coinbase EIP-5792 methods
# Search across all .test.ts and .test.tsx files for both wrapper and raw RPC method names
rg -l "sendCalls|getCallsStatus|getCapabilities|wallet_sendCalls|wallet_getCallsStatus|wallet_getCapabilities" -g "*.test.ts" -g "*.test.tsx"Length of output: 395 Add tests for Coinbase wallet’s EIP-5792 wrapper methods We have core EIP-5792 utilities covered under • sendCalls • getCallsStatus • getCapabilities Suggested location: 🧰 Tools🪛 GitHub Check: codecov/patch[warning] 281-302: packages/thirdweb/src/wallets/coinbase/coinbase-web.ts#L281-L302 [warning] 304-318: packages/thirdweb/src/wallets/coinbase/coinbase-web.ts#L304-L318 [warning] 320-337: packages/thirdweb/src/wallets/coinbase/coinbase-web.ts#L320-L337 🤖 Prompt for AI Agents |
||
| }; | ||
|
|
||
| return account; | ||
|
|
||
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.
🛠️ Refactor suggestion
Expand the changeset description to reflect the scope of changes.
The current description "Improve EIP5792 support" is too vague. Based on the PR changes, this should detail the specific improvements made, such as adding
sendCalls,getCallsStatus, andgetCapabilitiesmethods to wallet accounts and refactoring the EIP-5792 implementation.📝 Committable suggestion
🤖 Prompt for AI Agents