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

Adds permissioned signer support #637

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Adds permissioned signer support #637

wants to merge 5 commits into from

Conversation

kaw2k
Copy link
Contributor

@kaw2k kaw2k commented Feb 11, 2025

Description

This PR adds support for permissioned signers to the Aptos TS SDK.

Update: I added a unified permission class. You can see the diff of the change by looking at the commit history.

One design choice I would appreciate feedback on: The classes I created to represent permissions don't share a common parent. This impacts any wallet that needs to serialize requests before interfacing with nodes. For example, with aptos connect, this means that when requesting permissions from the wallet we need to separate the permission types in the request body. For example, here is how we request permissions in the dapp on aptos connect.

  const requestPermissionResponse = await dappClient.requestPermission({
    network,
    fungibleAssetPermissions: [
      new FungibleAssetPermission({
        amount: 100_000_000,
        asset: AccountAddress.A,
      }),
    ],
    gasPermission: new GasPermission({
      amount: 10_000_000,
    }),
  });

Note how the permissions are separated by type. If there was a common parent, we could have a single array of permissions like so:

const requestPermissionResponse = await dappClient.requestPermission({
  network,
  permissions: [
    new FungibleAssetPermission({
      amount: 100_000_000,
      asset: AccountAddress.A,
    }),
    new GasPermission({
      amount: 10_000_000,
    }),
  ],
});

Curious to hear opinions on this, if it is an issue or not.

Test Plan

There are a couple test cases I need to add after the issue with the script composer is resolved, mainly around the new gas permission. I have a little bit of cleanup to do in permissionedSigner.test.ts as well.

Outside of the test cases, the Aptos Connect PR has an e2e flow implemented for requesting permissions, and a dapp to consume the feature.

Related Links

  • There is a blocking issue related to Script Composer that is being tracked here. This PR will need to be rebased and fully tested once the issue is resolved.
  • An aptos connect PR that adds permissioned signer support.
  • A docs PR that adds documentation on the permissioned signer feature.

Checklist

  • Have you ran pnpm fmt?
  • Have you updated the CHANGELOG.md?

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