-
Notifications
You must be signed in to change notification settings - Fork 109
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
Refactor the account info output type to be compatible with AIP-62
standard
#441
base: main
Are you sure you want to change the base?
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,7 @@ | ||
--- | ||
"@aptos-labs/wallet-adapter-react": major | ||
"@aptos-labs/wallet-adapter-core": major | ||
"@aptos-labs/wallet-adapter-nextjs-example": major | ||
--- | ||
|
||
Refactor the account info output type to be compatible with AIP-62 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,8 @@ import { | |
InputGenerateTransactionPayloadData, | ||
AnyRawTransaction, | ||
Signature, | ||
AccountAuthenticator, | ||
AccountAddress, | ||
PublicKey, | ||
} from "@aptos-labs/ts-sdk"; | ||
import { WalletReadyState } from "../constants"; | ||
import { | ||
|
@@ -20,7 +21,10 @@ import { | |
AptosChangeNetworkMethod, | ||
AptosSignAndSubmitTransactionInput, | ||
} from "@aptos-labs/wallet-standard"; | ||
import { AptosStandardSupportedWallet } from "../AIP62StandardWallets/types"; | ||
import { | ||
AptosStandardSupportedWallet, | ||
StandardAccountInfoInput, | ||
} from "../AIP62StandardWallets/types"; | ||
|
||
export { TxnBuilderTypes, Types } from "aptos"; | ||
export type { | ||
|
@@ -55,6 +59,7 @@ export type WalletInfo = { | |
url: string; | ||
}; | ||
|
||
// Input type to handle legacy wallets that are not AIP-62 compatible | ||
export type AccountInfo = { | ||
address: string; | ||
publicKey: string | string[]; | ||
|
@@ -74,7 +79,7 @@ export declare interface WalletCoreEvents { | |
readyStateChange(wallet: Wallet): void; | ||
standardWalletsAdded(wallets: Wallet | AptosStandardSupportedWallet): void; | ||
networkChange(network: NetworkInfo | null): void; | ||
accountChange(account: AccountInfo | null): void; | ||
accountChange(account: AccountInfo | StandardAccountInfoInput | null): void; | ||
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. Do you think it might be better to just share the 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. This is tricky as the adapter still supports legacy and new standard types, so the account the adapter gets back from the wallet can be of both types. Changing it gets us into a type-rabbit-hole, so we simply change the actual input type the adapter returns. In the near future, we should probably support only AIP-62 wallets 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. I think we should use If I understand correctly, this is an event that can be listened to externally by the dapp. In case of legacy plugins, we should be able to adapt the return types to EDIT: here's a good spot where we could normalize |
||
} | ||
|
||
export interface SignMessagePayload { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,14 +8,13 @@ import { | |
Ed25519PublicKey, | ||
InputGenerateTransactionOptions, | ||
Ed25519Signature, | ||
AptosConfig, | ||
InputSubmitTransactionData, | ||
PendingTransactionResponse, | ||
Aptos, | ||
generateRawTransaction, | ||
SimpleTransaction, | ||
NetworkToChainId, | ||
Hex, | ||
PublicKey, | ||
} from "@aptos-labs/ts-sdk"; | ||
import EventEmitter from "eventemitter3"; | ||
import { | ||
|
@@ -60,7 +59,6 @@ import { | |
WalletName, | ||
WalletCoreV1, | ||
CompatibleTransactionOptions, | ||
convertNetwork, | ||
convertPayloadInputV1ToV2, | ||
generateTransactionPayloadFromV1Input, | ||
} from "./LegacyWalletPlugins"; | ||
|
@@ -80,6 +78,8 @@ import { | |
WalletStandardCore, | ||
AptosStandardSupportedWallet, | ||
AvailableWallets, | ||
StandardAccountInfoInput, | ||
AccountInfoOutput, | ||
} from "./AIP62StandardWallets"; | ||
import { GA4 } from "./ga"; | ||
import { WALLET_ADAPTER_CORE_VERSION } from "./version"; | ||
|
@@ -131,7 +131,7 @@ export class WalletCore extends EventEmitter<WalletCoreEvents> { | |
private _wallet: Wallet | null = null; | ||
|
||
// Current connected account | ||
private _account: AccountInfo | null = null; | ||
private _account: AccountInfo | StandardAccountInfoInput | null = null; | ||
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. Same here, let's just use |
||
|
||
// Current connected network | ||
private _network: NetworkInfo | null = null; | ||
|
@@ -465,7 +465,7 @@ export class WalletCore extends EventEmitter<WalletCoreEvents> { | |
* @param account An account | ||
*/ | ||
private ensureAccountExists( | ||
account: AccountInfo | null | ||
account: AccountInfo | StandardAccountInfoInput | null | ||
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. same here |
||
): asserts account is AccountInfo { | ||
if (!account) { | ||
throw new WalletAccountError("Account is not set").name; | ||
|
@@ -541,57 +541,12 @@ export class WalletCore extends EventEmitter<WalletCoreEvents> { | |
* Sets the connected account | ||
* | ||
* `AccountInfo` type comes from a legacy wallet adapter plugin | ||
* `StandardAccountInfo` type comes from AIP-62 standard compatible wallet when onAccountChange event is called | ||
* `UserResponse<StandardAccountInfo>` type comes from AIP-62 standard compatible wallet on wallet connect | ||
* `StandardAccountInfo` type comes from AIP-62 standard compatible wallet | ||
* | ||
* @param account An account | ||
*/ | ||
setAccount( | ||
account: | ||
| AccountInfo | ||
| StandardAccountInfo | ||
| UserResponse<StandardAccountInfo> | ||
| null | ||
): void { | ||
if (account === null) { | ||
this._account = null; | ||
return; | ||
} | ||
|
||
// Check if wallet is of type AIP-62 standard | ||
if (this._wallet?.isAIP62Standard) { | ||
// Check if account is of type UserResponse<StandardAccountInfo> which means the `account` | ||
// comes from the `connect` method | ||
if ("status" in account) { | ||
const connectStandardAccount = | ||
account as UserResponse<StandardAccountInfo>; | ||
if (connectStandardAccount.status === UserResponseStatus.REJECTED) { | ||
this._connecting = false; | ||
throw new WalletConnectionError("User has rejected the request") | ||
.message; | ||
} | ||
// account is of type | ||
this._account = { | ||
address: connectStandardAccount.args.address.toString(), | ||
publicKey: connectStandardAccount.args.publicKey.toString(), | ||
ansName: connectStandardAccount.args.ansName, | ||
}; | ||
return; | ||
} else { | ||
// account is of type `StandardAccountInfo` which means it comes from onAccountChange event | ||
const standardAccount = account as StandardAccountInfo; | ||
this._account = { | ||
address: standardAccount.address.toString(), | ||
publicKey: standardAccount.publicKey.toString(), | ||
ansName: standardAccount.ansName, | ||
}; | ||
return; | ||
} | ||
} | ||
|
||
// account is of type `AccountInfo` | ||
this._account = { ...(account as AccountInfo) }; | ||
return; | ||
setAccount(account: AccountInfo | StandardAccountInfo | null): void { | ||
this._account = account; | ||
} | ||
Comment on lines
+548
to
550
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. This would be a good spot where we support both |
||
|
||
/** | ||
|
@@ -687,9 +642,30 @@ export class WalletCore extends EventEmitter<WalletCoreEvents> { | |
* @return account info | ||
* @throws WalletAccountError | ||
*/ | ||
get account(): AccountInfo | null { | ||
get account(): AccountInfoOutput | null { | ||
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. Let's just return |
||
try { | ||
return this._account; | ||
if (!this._account) return null; | ||
return { | ||
address: | ||
typeof this._account.address === "string" | ||
? AccountAddress.from(this._account.address) | ||
: this._account.address, | ||
publicKey: | ||
// for backward compatibility, if the account public key is of type `string` convert it to Ed25519PublicKey | ||
typeof this._account.publicKey === "string" | ||
? new Ed25519PublicKey(this._account.publicKey) | ||
GhostWalker562 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
: // for backward compatibility, if the account public key is of type `string[]` convert each string to Ed25519PublicKey | ||
Array.isArray(this._account.publicKey) && | ||
this._account.publicKey.every( | ||
(item) => typeof item === "string" | ||
) | ||
? this._account.publicKey.map((key) => new Ed25519PublicKey(key)) | ||
Comment on lines
+657
to
+662
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. Ideally we should convert |
||
: // else, the account is of the new standard and return it with the exact type | ||
(this._account.publicKey as PublicKey), | ||
minKeysRequired: | ||
(this._account as AccountInfo).minKeysRequired ?? undefined, | ||
ansName: this._account.ansName, | ||
}; | ||
} catch (error: any) { | ||
throw new WalletAccountError(error).message; | ||
} | ||
|
@@ -931,7 +907,7 @@ export class WalletCore extends EventEmitter<WalletCoreEvents> { | |
if (this._wallet.signTransaction) { | ||
// If current connected wallet is AIP-62 standard compatible | ||
// we want to make sure the transaction input is what the | ||
// standard expects, i,e new sdk v2 input | ||
// standard expects, i.e new sdk v2 input | ||
if (this._wallet.isAIP62Standard) { | ||
// if rawTransaction prop it means transaction input data is | ||
// compatible with new sdk v2 input | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import { AccountAddress } from "@aptos-labs/ts-sdk"; | ||
import { AptosStandardWallet } from "../AIP62StandardWallets"; | ||
import { WalletInfo } from "../LegacyWalletPlugins"; | ||
import { AnyAptosWallet } from "../WalletCore"; | ||
|
@@ -44,9 +45,9 @@ export function isInstallRequired(wallet: AnyAptosWallet) { | |
} | ||
|
||
/** Truncates the provided wallet address at the middle with an ellipsis. */ | ||
export function truncateAddress(address: string | undefined) { | ||
export function truncateAddress(address: AccountAddress | undefined) { | ||
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. small nit: we could use |
||
if (!address) return; | ||
return `${address.slice(0, 6)}...${address.slice(-5)}`; | ||
return `${address.toString().slice(0, 6)}...${address.toString().slice(-5)}`; | ||
} | ||
|
||
/** Returns `true` if the provided wallet is an Aptos Connect wallet. */ | ||
|
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.
let's use
AccountInfo
from the standard