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

Upgrade Aptos to latest Aptos Wallet Adapter version #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0xmaayan
Copy link

@0xmaayan 0xmaayan commented Feb 20, 2025

Aptos Wallet Adapter has been major upgraded to support only wallets that follow the official Apos wallet standard, see aptos-labs/aptos-wallet-adapter#464

This PR updates the @xlabs-libs/wallet-aggregator-aptos to be up to date with the latest Aptos wallet adapter, as well as bumps other Aptos dependencies versions.

This is important because

  • We want to make sure users are not interacting with out-of-date and unmaintained wallets
  • Get new features and updates coming to the Aptos Wallet Adapter
  • Better handling of the types coming from the wallet for a better dev exp.

You can see latest version deployed here https://aptos-labs.github.io/aptos-wallet-adapter/

Note: looks like the @xlabs-libs/wallet-aggregator-aptos does not follow semver? I'd assume this type of change requires a major bump, but will be happy to bump to whatever you think makes sense.

Also, if you have a test plan I can follow, I'd be happy to

Copy link

XLabs/wallet-aggregator-sdk #76

Change Summary:

  • Upgraded Aptos wallet SDK dependencies to newer versions, including @aptos-labs/ts-sdk, @aptos-labs/wallet-adapter-core, and @aptos-labs/wallet-standard
  • Simplified wallet core factory method by removing hardcoded wallet additions and making it more flexible
  • Refactored the AptosWallet class to use new wallet adapter interfaces and types
  • Removed several deprecated or non-functional wallet adapter imports
  • Updated type definitions and method signatures to match the latest SDK standards

Risk Score: 3

  • Explanation: Mostly a dependency upgrade and code refactoring with minimal intrusive changes. The modifications appear to improve type safety and flexibility without introducing significant new risks.

Potential Vulnerability:
None identified in this PR. The changes appear to be focused on upgrading dependencies and improving type definitions.

Code Smell:
File: packages/wallets/aptos/src/aptos.ts:24-36

// Removed multiple wallet imports which suggests previous code might have been including non-functional or deprecated adapters
// Previous implementation was including wallets that were commented out or not working
  • Explanation: The previous implementation was including commented-out wallet adapters, which indicates potential technical debt and unclear development strategy.

Debug Log:
None identified in the provided diff.

Unintended Consequences:
File: packages/wallets/aptos/src/aptos.ts:95-120

async connect(): Promise<string[]> {
  await this.walletCore.connect(this.selectedAptosWallet.name);

  // set account
  this.account = this.walletCore.account;

  // Set address
  this.address = this.account?.address.toString();
  this.walletCore.on(
    "accountChange",
    async (accountInfo: AccountInfo | null) => {
      this.account = accountInfo;
      this.address = accountInfo?.address.toString();
    }
  );
  // ... rest of the method
}
  • Explanation: The new implementation changes how account and address are set. While not necessarily a vulnerability, it might cause unexpected behavior for existing integrations expecting the previous method of handling wallet connections.

Additional Observation:
The PR shows a deliberate effort to clean up and modernize the Aptos wallet adapter, removing unnecessary complexity and aligning with the latest SDK standards. The changes demonstrate a commitment to maintaining clean, up-to-date code.

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.

1 participant