Skip to content
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

Web3Modal API v2 spec #228

Closed
wants to merge 809 commits into from
Closed

Web3Modal API v2 spec #228

wants to merge 809 commits into from

Conversation

bkrem
Copy link
Member

@bkrem bkrem commented May 6, 2024

Context

See: https://www.notion.so/walletconnect/Web3Modal-API-v2-Proposal-a2fc6323156d49f79d8092f5c39b8cda

Scope

  • Problem 4 & 5 from the proposal are out-of-scope for the spec, since they are implementation details.

TODO

  • Move current (v1) spec into a separate markdown file where we can back-reference it
  • Account for the endpoints that were added previously without updating the v1 spec
  • Finalize spec

chris13524 and others added 30 commits October 17, 2023 09:01
…ch-subscriptions

chore: clarify multiple watchSubscriptions
…ion.appAuthenticationKey

feat: NotifyServerSubscription.appAuthenticationKey
…egistration

chore: remove Push Server registration
Co-authored-by: Bartosz Rozwarski <[email protected]>
…ructure

fix(blockchain-api): adding `json-rpc` endpoint spec and removing chain-specific rpc descriptions
@bkrem bkrem force-pushed the feat/w3m-api-v2-spec branch from 3eead4a to f349179 Compare May 30, 2024 14:17
@bkrem bkrem requested a review from chris13524 May 30, 2024 14:18
@bkrem
Copy link
Member Author

bkrem commented May 30, 2024

@chris13524 I added an affordance for granular image size control as discussed/added by you to the proposal doc 👍

- Options: `w3m | wcm`
- Example: `st=w3m`
- `sv=`: SDK version
- Options: `${framework}-${library}-${version}`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to have a new format for this when we introduce the multichain refactor as it might include multiple libraries. If a user decides to use SolanaWeb3JsAdapter AND EVMWagmiAdapter then what would library have?

We could maybe include an extra query param libraries ? What would be best for analytics also cc @dnul

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From AppKit versions that support multi-chain onwards, could we do e.g. react-solanaweb3js;evmwagmi-5.0.0, where the library parameter can be a ;-separated list (or via some other separator)? Maybe will relabel to {adapters} given this.

We could maybe include an extra query param libraries ?

I suspect introducing a separate parameter will create more complicated parsing:

  • need to check for the positional library value still for implementations up to version X that have sv= as${framework}-${library}-${version}
  • need to check for explicit libs= query param for implementations beyond version X, where sv= is only ${framework}-${version}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's not overcomplicate things. I believe the ; separated values would work (though they def look ugly 😭 ).

All API endpoints expect the following mandatory query parameters:

- `st=`: SDK type
- Options: `w3m | wcm`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add appkit here?

"homepage": string,
"image_id": string,
"order": number,
"mobile_link": string | null,
Copy link

@ignaciosantise ignaciosantise Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recently we had a case in web were we needed the universal_link only, nowadays the api returns the deeplink by default, and universal_link as a fallback.

Should we consider sending both again?
https://github.com/WalletConnect/web3modal/pull/2352/files

Copy link
Member Author

@bkrem bkrem Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes definitely 👍 I think the API shouldn't be internalising this logic and the clients should be deciding on which one to use.

If we only want to use deep_link happy days, but it shouldn't be returning ambiguous values (sometimes deep link sometimes universal) where clients have no choice in the matter.

Copy link

@ignaciosantise ignaciosantise Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like this?

"mobile_link": {
   "deep_link": string | null,
   "universal_link": string | null
} | null

Copy link
Member Author

@bkrem bkrem Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we should basically mirror what was already the standard in Explorer API, e.g.

"mobile": {
  "native": "trust://",
  "universal": "https://link.trustwallet.com"
},

But we can keep the deep_link/universal_link keys and not go back to native/universal

One less difference to deal with between the response schemas that way.

Will incorporate into the new PR 👍

"mobile_link": string | null,
"desktop_link": string | null,
"webapp_link": string | null,
"app_store": string | null,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are gonna send platform: ios|android param, should we need both app_store and play_store values in the response object? I mean, if I request platform: ios there's no point for me to have play_store URL, right?

Copy link
Member Author

@bkrem bkrem Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, so:

  • by default it still returns both since the platform param is optional
  • if platform is provided we could narrow the response down to omit what isn't needed for platform

@chris13524 chris13524 closed this Jun 6, 2024
@chris13524 chris13524 force-pushed the feat/w3m-api-v2-spec branch from 453ade4 to fcc664a Compare June 6, 2024 19:57
@bkrem bkrem mentioned this pull request Jun 6, 2024
4 tasks
@bkrem
Copy link
Member Author

bkrem commented Jun 6, 2024

Superseded by #239

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.