Web3Modal API v2 spec #239
Conversation
Co-authored-by: Celine Sarafa <[email protected]>
Co-authored-by: Celine Sarafa <[email protected]>
Co-authored-by: Celine Sarafa <[email protected]>
Co-authored-by: Celine Sarafa <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
bkrem
requested review from
tomiir,
quetool,
skibitsky,
enesozturk,
ignaciosantise,
Cali93 and
svenvoskamp
|
It'd be great to get an answer to this question. |
|
Hello, since we are moving WalletConnectModal to AppKit we need a way to filter wallets that support the WalletConnect protocol. |
@glitch-txs but this doesn't require explicit breaking changes to the API I presume? Seems like either a dedicated endpoint or an additional query parameter so wouldn't have to be settled right now for us to move ahead on implementing the v2 spec. |
|
A query parameter would be great |
|
|
||
| #### Route Parameters | ||
|
|
||
| - `connector_id`: Descriptive name for the connector (e.g., `coinbase-wallet`) |
There was a problem hiding this comment.
are we using well-known names for these e.g. their rdns? else how do we get the list of available connectors? do we need to hardcode it ?
There was a problem hiding this comment.
Agree with Rocky here we should use the rdns
There was a problem hiding this comment.
Agreed, RDNS makes a lot more sense. Will update 👍
There was a problem hiding this comment.
@tomiir I just noticed that a number of connectors that are currently being mapped here in PresetsUtil (e.g. Safe, Ledger) don't have rdns values set in Cloud and hence the /getWallets response.
Wouldn't this then still necessitate manual mappings in the client either way?
There was a problem hiding this comment.
Should the Coinbase example be coinbaseWalletSDK? https://github.com/wevm/wagmi/blob/61483838ec238b406676004a95d08ff702d750d9/packages/connectors/src/coinbaseWallet.ts#L90
There was a problem hiding this comment.
What's the resolution here ? @tomiir
There was a problem hiding this comment.
Coinbase we use coinbaseWalletSDK as Chris mentions, but you are correct @bkrem that Ledger, Safe and other wallets that are not 6963 might not have it :|
I think there's no way around the id list then, unless there's a known global ID we can use instead :(
We'd still need to somehow fetch the id, I guess we already have it from requesting all wallets before
There was a problem hiding this comment.
I think there's no way around the id list then, unless there's a known global ID we can use instead
Yeah the issue at hand seems to be that there's no well-known identifier for these connectors.
We'd still need to somehow fetch the id
They're all defined here already though right? https://github.com/WalletConnect/web3modal/blob/main/packages/scaffold-utils/src/ConstantsUtil.ts
The only immediate improvement we could make here from what I can see is what is already reflected in the spec, i.e. remove a layer of mapping and query the API by these word-based connectorIDs directly instead of having this mapping to the imageId on the client/appkit side: https://github.com/WalletConnect/web3modal/blob/main/packages/scaffold-utils/src/PresetsUtil.ts#L78
It means that the mapping (e.g. coinbaseWalletSDK -> imageId) needs to live in the API but we already effectively have this burden of maintenance from allowlisting these connectors' imageIds so they can be queried: https://github.com/WalletConnect/cloud-app/blob/develop/services/web3modal-api/src/tools/ConstantsTool.ts#L99
|
|
||
| ### `GET /v2/asset-image/token/{size}/{token_symbol}` | ||
|
|
||
| Fetches image for known tokens using the token's ticker symbol. |
There was a problem hiding this comment.
Isn't it possible for ticker symbols to overlap? Maybe there is a more robust way to do this? E.g. chain ID + token ID? How is swaps doing this right now?
There was a problem hiding this comment.
I agree we at least need to scope this by network given ticker might repeat (eg OP ETH vs Mainnet ETH)
.../token/{size}/{caip-2}/{tokenAddress} would be the most exact, .../token/{size}/{caip-2}/{symbol} seems more similar to current structure
There was a problem hiding this comment.
Good points 👍
One question: will requiring tokenAddress for querying cause more lookup requirements on the SDK side (which we want to explicitly avoid with these changes) or can you query /{caip-2}/{tokenAddress} a priori @tomiir?
How is this being queried in the clients currently with the existing /public/getTokenImage/ETH approach?
There was a problem hiding this comment.
@Cali93 was just checking for implementations.
This was originally intended for token images from Onramp activity- The address is not avail on the response from the tx status https://docs.cdp.coinbase.com/onramp/docs/api-reporting/#transaction-status
but checked the API specs and the response from the options returns the contract address already, so we should have that information available if we set it up correctly
https://docs.cdp.coinbase.com/onramp/docs/api-configurations/#example-requestresponse-1
Given we are requesting this from blockchain API, would it make sense to remove this completely and have blockchain API handle image formatting for that endpoint?
Other integrations that use token images are not using this endpoint afaik
Cc @svenvoskamp @enesozturk any cases you can recall where this is being fired? couldn't find anything on the code
There was a problem hiding this comment.
I was mainly considering a portfolio view; looks like the balances endpoint returns the contract address.
There was a problem hiding this comment.
I propose we use CAIP-19 for this endpoint. So it would look something like this for DAI:
GET /v2/asset-image/token/64x64/eip155%3A1%2Ferc20%3A0x6b175474e89094c44da98b954eedeac495271d0f`
This approach has the added benefit of including the token ID which is important for ERC-1155 tokens as well as NFTs. I.e. having multiple assets for a given contract address. ERC-1155 supersedes ERC-20 and ERC-721 in function.
There was a problem hiding this comment.
Nice, didn't know of CAIP19. It seems like the most explicit given we get the contract address from the API @chris13524
There was a problem hiding this comment.
Nice one on the CAIP-19 suggestion @chris13524 💯 🙏
One technicality we need to resolve, without explicit URL-encoding the DAI example is:
eip155:1/erc20:0x6b175474e89094c44da98b954eedeac495271d0f
The slash will be interpreted as an additional path segment, and we can't expect clients to do partial URL-encoding, so we either:
- replace the
/with another unambiguous value - Treat the values before and after the
/like real path segments, i.e.GET /v2/asset-image/token/{chain_id}/{asset_namespace + ":" + asset_reference}
Personally I'd prefer to just replace with e.g. an underscore (eip155:1_erc20:0x6b175474e89094c44da98b954eedeac495271d0f) because using part of the identifier as a path seems odd.
There was a problem hiding this comment.
we can't expect clients to do partial URL-encoding
@bkrem what do you mean here? URL encoding is just as easy as base64 encoding. For example in JS with a function such as encodeURIComponent():
`GET /v2/asset-image/token/64x64/${encodeURIComponent(caip19Id)}`Keeping it in the original CAIP-19 format sounds simpler to me as you can easily pass the value to another CAIP-19-compatible service or use a lookup table w/o needing to recombine various pieces and use custom encoding.
There was a problem hiding this comment.
URL encoding is just as easy as base64 encoding. For example in JS with a function such as encodeURIComponent()
Sure I get the approach, but I think it's odd to expect that kind of accounting work from a client. Seems like this would already get awkward when you want to query this endpoint with e.g. curl.
Co-authored-by: Chris Smith <[email protected]>
Co-authored-by: Chris Smith <[email protected]>
There was a problem hiding this comment.
Can we specify caching behavior for these endpoints? I propose:
- response:
Cache-Control: public, max-age=43200(same as current) - response:
ETag: <hash of content> - request:
If-None-Match: <value of ETag header>
The request will return a 304 Not Modified if the hash matches, saving bandwidth needing to respond with duplicate data, and renewing the Age of the resource.
Yeah would be great to add this as an optimisation. Also it's non-breaking so not strictly a v2 issue, we could in theory already do this right now and clients update their fetch behaviour. @Cali93 any objections to the |
There was a problem hiding this comment.
Thanks for driving this 🙏
Really looking forward to the implementation of this and the results that it'll yield.
No objection for the additional cache headers. Great suggestions from @chris13524 !
| "search": string | null, // Optional search term (e.g., `MetaMask`) | ||
| "include": string | null, // Optional comma-separated list of wallet IDs to include | ||
| "exclude": string | null, // Optional comma-separated list of wallet IDs to exclude | ||
| "chains": string | null, // Optional comma-separated list of chain identifiers (e.g., `eip155:1`) |
There was a problem hiding this comment.
We should also document the behaviour to improve DX when they provide these query params.
Eg what's the logical operator used underneath, what happens if you provide exclude and search for an excluded walletId, etc.
|
|
||
| #### Route Parameters | ||
|
|
||
| - `connector_id`: Descriptive name for the connector (e.g., `coinbase-wallet`) |
There was a problem hiding this comment.
What's the resolution here ? @tomiir
| { | ||
| "id": string, | ||
| "name": string, | ||
| "homepage": string, | ||
| "image_id": string, | ||
| "order": number, | ||
| "desktop_link": string | null, | ||
| "webapp_link": string | null, | ||
| "app_store": string | null, | ||
| "play_store": string | null, | ||
| "mobile": { | ||
| deep_link: string | null, | ||
| universal_link: string | null | ||
| } | ||
| "injected": [ | ||
| { | ||
| "namespace": string, | ||
| "injected_id": string | ||
| } | ||
| ] | null | ||
| } |
There was a problem hiding this comment.
Do we need ALL of this information in all cases?
Can we shorten the names of the fields to save on bandwidth?
Can we supply as a parameter what platform the app is on and only return the Apple, Google, or mobile links accordingly?
Can we request mobile links/etc. when the wallet is clicked instead of in the initial download?
Co-authored-by: Cali <[email protected]>
Co-authored-by: Cali <[email protected]>
|
|
||
| ```typescript | ||
| // Maps EIP155 chain IDs -> `asset_id` | ||
| const EIP155NetworkImageIds = { |
Context
mainbase branch)See: https://www.notion.so/walletconnect/Web3Modal-API-v2-Proposal-a2fc6323156d49f79d8092f5c39b8cda
Scope
TODO