-
Notifications
You must be signed in to change notification settings - Fork 118
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
New wallet adapter interface #473
base: main
Are you sure you want to change the base?
Conversation
cbce48d
to
1ad431a
Compare
1ad431a
to
a5a2f67
Compare
a5a2f67
to
9657593
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.
Nice refactoring and thinking. How hard would it be to rebase it with #464 ?
Some Qs
- I dont see any mobile specific handling
- I feel some implementations can easily live in the standard repo, WalletAdapter interface for example
- We need to think how these changes go with other workstream we have for the cross chain adapter
* A wallet instance adapted from an Aptos standard wallet that supports | ||
* all required features with minimum version. | ||
*/ | ||
export class AdaptedWallet { |
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.
This can probably live in the wallet-standard repo, I actually have a long standing open PR for that aptos-labs/wallet-standard#7
readonly availableNetworks: Network[]; | ||
|
||
// Google Analytics 4 module | ||
private readonly ga4?: GA4; |
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.
why this needs to be part of the AdapterWallet class? it is used for the wallet-core product tracking
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.
Happy to move it elsewhere! Where were you thinking of putting this?
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.
hmm this is tricky, the purpose of it is to record wallet events like connect, disconnect, signAndSubmitTransaction, etc.
So it makes sense to keep it here and record each AdaptedWallet
method.
But it also means we create a ga4
instance for each AdaptedWallet
https://github.com/aptos-labs/aptos-wallet-adapter/pull/473/files#diff-f9f367b57dfbc51263db65e5ad66417c80df2a0a83a21200c5a1eeb48988d3ceR39 and that feels unnecessary.
Maybe we can initiate it in the WalletAdapter
class, and pass it to the AdaptedWallet
instance to be used in each wallet function
private recordEvent(eventName: string, additionalInfo?: object) { | ||
this.ga4?.gtag("event", `wallet_adapter_${eventName}`, { | ||
wallet: this.name, | ||
// network: this._network?.name, |
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.
why comment? it is actually very helpful for our metrics
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.
We don't have direct access to the active network here at the moment.
I can add it, it just adds a bit of complexity.
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.
hmm we have it in the provider useActiveWallet
, maybe we can get it from there?
readonly onAccountConnectedListeners = new Set<(account: AccountInfo) => void>(); | ||
readonly onAccountDisconnectedListeners = new Set<() => void>(); | ||
|
||
async getConnectedAccounts(): Promise<AccountInfo[]> { |
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.
why do we want to represent it as an array? I believe aptos connect is the only wallet that supports multiple accounts?
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 think that more generally, we should distinguish between the concept of "connected account" and "active account".
At the moment, Petra only supports requesting signatures for the active account, but technically nothing prevents the dapp from requesting a signature by a different connected account (or even network).
We just decided that it's "safer" this way.
As you mentioned, Aptos Connect allows specifying the signer address and network, and I want to be able to allow wallets to do that.
* Required features with minimum versions. | ||
* In the future, we might choose to slowly deprecate older versions to simplify the adapter's code. | ||
*/ | ||
const requiredFeatures: [name: keyof MinimallyRequiredFeatures, version: TargetVersion][] = [ |
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.
maybe put in the wallet-standard repo, to ease on maintenance
|
||
export interface WalletAdapterConfig { | ||
disableTelemetry?: boolean; | ||
whitelist?: 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.
what are these lists for?
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.
By default, all wallets implementing the standard will be considered.
These list allow the dapp developer to selectively ignore unwanted wallets, or explicitly defining which wallets are supported.
The check is done by name
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.
Maybe we can use a stronger type? something like what we have now
type AvailableWallets = "Petra" | "Pontem" | "Nightly" | ....
whitelist?: AvailableWallets[]
@@ -0,0 +1,264 @@ | |||
import { |
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.
Part of me really wants to stop legacy plugin supports, at this point we have been given enough time for wallets to migrate and all major wallets are now on AIP-62. Our metrics also show that.
Deprecate legacy wallets will also make sure no new wallets coming to the ecosystem implemented with legacy flow in mind.
See more context https://aptos-org.slack.com/archives/C06TWPH09Q9/p1738169671855909?thread_ts=1721771598.556349&cid=C06TWPH09Q9
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.
Yeah I support this. I wouldn't mind removing this file altogether.
It's a nice example on how to adapt other objects into standard wallets. (e.g. cross-chain wallets)
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.
interesting.... then maybe we can put it under an example
folder? I think for cross-chain wallets we will probably manage it in the cross-chain package
import { useAvailableWallets } from './useAvailableWallets'; | ||
|
||
export interface UninitializedWallet { | ||
isInitialized: false; |
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.
what does initialize/uninitialize mean?
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.
In order for the wallet to be "usable" aka "initialized", we have to fetch some information e.g. activeAccount
and activeNetwork
. This step is async so the wallet is going to be in a "loading" state for a couple frames.
The possible states are:
- uninitialized (we can potentially show a spinner in the UI)
- intialized, not connected
- initialized, connected
We can add a caching logic later, but figured we should keep it simple for now :)
features: AptosFeatures; | ||
disconnect: () => Promise<void>; | ||
signMessage: (input: AptosSignMessageInput) | ||
=> ReturnType<AdaptedWallet['signMessage']>; |
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.
TIL
return window.localStorage.getItem(activeWalletIdStorageKey) ?? undefined; | ||
} | ||
|
||
export function setActiveWalletId(walletId: string | undefined) { |
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.
walletId
can probably just be the wallet name
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.
Yes, at the moment the name is effectively the identifier.
I was hoping to differentiate between "id" and "display name", so we can have:
{ id: 'petra', name: "Petra", ... }
{ id: 'aptos-connect-google', name: "Continue with Google", ... }
{ id: 'aptos-connect-apple', name: "Continue with Apple", ... }
but I also recognize there isn't a functional reason to differentiate at the moment.
It just "feels" right haha
true, mobile specific handling and registry wallets have not been implemented yet
Yes that's true. I added the code here to keep the changes in one spot, but it can be under the standard's package.
Yes agreed, if you have any doc or ongoing conversations I'd be happy to take a look to make sure these changes are compatible and take into account the cross-chain adapter |
Not hard at all! |
9657593
to
8db2718
Compare
Writing here my thoughts and post-discussion agreements
This changes can be then combined with other workstream we have for the wallet adapter and be released with a major upgrade |
Agreed! |
/** | ||
* Custom network configuration. Requires at least the full node URL to work properly. | ||
*/ | ||
export interface CustomNetwork { |
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.
this should be aligned with the AptosConfig
instance in the sdk, right?
https://github.com/aptos-labs/aptos-ts-sdk/blob/main/tests/unit/aptosConfig.test.ts#L71
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.
Yes, ideally
I wanted to make the network definition a bit more expressive i.e.
- when using standard networks you just pass the network name as shorthand, everything else will be inferred for you
- when using a custom network, you have to pass it in object form, and some fields are required
at the moment, in the SDK everything other than network
is optional so you can pass a standard network with overrides, or a custom network with missing fields
if someone is using mainnet
, testnet
... I don't think there's a big use-case for overriding values.
That's what a "custom" network would be. But we can still allow that if needed
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.
when using a custom network, you have to pass it in object form, and some fields are required
Yep, but dont we want to have then pass in an object similar to the one that will eventually be used with the SDK to initiate an Aptos connection?
if (isFeatureMinorVersion(feature, "1.0")) { | ||
const { signerAddress, feePayer } = input; | ||
// This will throw an error if it requires an async call | ||
const transaction = buildTransaction(input); |
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.
hmm looks like we are now requiring the dapp to provide the account sequence 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.
[email protected]
doesn't support payload inputs as arguments.
If the wallet doesn't implement [email protected]
and the dapp is passing a payload input, we try our best to make it work.
Better to have the wallet adapter give you an expressive and actionable error, than having the popup not opening on safari :D
I also thought of allowing wallets to specify whether they're ok with async calls through a flag.
For example, chrome extensions don't have that limitations. Aptos Connect does.
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 also thought of allowing wallets to specify whether they're ok with async calls through a flag.
For example, chrome extensions don't have that limitations. Aptos Connect does.
That could be interesting - I assume since AC has this limitation, other SDK wallets also have it
@@ -0,0 +1,10 @@ | |||
export function isFeatureMinorVersion< |
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'd move it to the standard repo
My proposal for a new interface of the Aptos Wallet Adapter.
Key objectives:
Changes in
core
package:AdaptedWallet
class, that will take as an input a wallet interface with minimum required features and expose a unified and clean interface for accessing it. Internally, this class performs feature version negotiation, ensuring we're using the most up-to-date version or fallback to older but still compatible versions.WalletAdapter
).walletFromLegacyPlugin
so that dapps that rely on legacy plugins can still use the new interface (all required features need to be supported)Changes in
react
package:useActiveWallet
has different states. Union discrimination enables intuitive and expressive access to features based on the current state.example:
TODO:
react
package and showcase it's expressivenesss