Permissioned Accounts #594

chiumax · 2024-11-25T22:07:36Z

Description

big thanks to @kaw2k for the help + stubs

Best Practice Questions:

Currently types are living under internal/Permissions.ts, should they be moved to api/...ts?
- Based on the context, types should likely move to api/ since they are part of the public interface
- This matches the pattern seen in api/permissions.ts which exposes the Permission types
Should we remove the factories for each permissions or keep both types and factories?
- Factories don't provide much value beyond type construction
- Tests show direct object construction is just as clear
- Consider deprecating factories in favor of just types

Points of Interest:

Issue with returning arrays in grantPermission:
- NFT permissions can have multiple capabilities (transfer, mutate)
- Current workaround is handling each capability separately
- Need to investigate why batched array returns break Move code
- Consider refactoring NFT permissions into separate permission objects per capability
- Gas vs APT permissions?

Architecture Changes @kaw2k:

Moved core permission functionality from tests to internal/Permissions.ts
Updated some of the syntax in the test cases
Added proper abstraction layers:
- api/permissions.ts - Public interface
- internal/permissions.ts - Implementation
- tests - Integration testing

Permission Types:

FungibleAssetPermission
Gas Permission
NFT Permission
NFT Collection Permission

Core Features:

Request Permission
- FungibleAsset ✅
- NFT 🟠
  - mutate ❌
  - 'transfer` ✅
- Gas ❌
- NFTCollection ❌
Revoke Permission
- FungibleAsset ✅
- NFT ✅
- Gas ❌
- NFTCollection ❌
View Permission
- FungibleAsset ✅
- NFT 🟠
  - mutate ❌
  - transfer ✅
- Gas ❌
- NFTCollection ❌

TODO:

Implement NFT mutate capability
Investigate batched permission returns
Add proper error handling
Consider deprecating permission factories

Test Plan

Checklist

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

chiumax · 2024-11-26T18:55:51Z

src/internal/permissions.ts

+        ],
+        typeArguments: [],
+      });
+    case PermissionType.NFT: {


returning an array here breaks

chiumax · 2024-11-26T18:56:20Z

src/internal/permissions.ts

+export type RevokeNFTPermission = Pick<NFTPermission, "type" | "assetAddress">;
+
+// factories -- on second thought, i think these are dumb
+export function FungibleAssetPermission(args: Omit<FungibleAssetPermission, "type">): FungibleAssetPermission {


remove these?

kaw2k · 2024-11-26T18:19:46Z

src/internal/permissions.ts

+export enum MoveVMPermissionType {
+  FungibleAsset = "0x1::fungible_asset::WithdrawPermission",
+
+}


I think this would technically be a Capability not a permission. Maybe we have the capability types here and for nfts be the actual move vm string so we can re-use them? Need to think on that a moment.

kaw2k · 2024-11-26T18:22:38Z

src/internal/permissions.ts

+export interface GenericPermission {
+  type: Exclude<string, PermissionType | `${PermissionType}`>;
+  [key: string]: string | Exclude<string, PermissionType | `${PermissionType}`>;
+}


Placeholder: Is this needed?

kaw2k · 2024-11-26T18:23:45Z

src/internal/permissions.ts

+export function GenericPermission(args: GenericPermission): GenericPermission {
+  return {...args}
+}


Maybe move to the test file then.

kaw2k · 2024-11-26T18:24:07Z

src/internal/permissions.ts

+export interface PermissionTemp{
+  asset: string;
+  type: string;
+  remaining: string;
+}


kaw2k · 2024-11-26T19:01:25Z

src/internal/permissions.ts

+// factories -- on second thought, i think these are dumb
+export function FungibleAssetPermission(args: Omit<FungibleAssetPermission, "type">): FungibleAssetPermission {
+  return { type: PermissionType.FungibleAsset, ...args };
+}
+export function GasPermission(args: Omit<GasPermission, "type">): GasPermission {
+  return { type: PermissionType.Gas, ...args };
+}
+export function NFTPermission(args: Omit<NFTPermission, "type">): NFTPermission {
+  return { type: PermissionType.NFT, ...args };
+}
+export function NFTCollectionPermission(args: Omit<NFTCollectionPermission, "type">): NFTCollectionPermission {
+  return { type: PermissionType.NFTCollection, ...args };
+}
+
+// revoke factories
+export function RevokeFungibleAssetPermission(args: Omit<RevokeFungibleAssetPermission, "type">): RevokeFungibleAssetPermission {
+  return { type: PermissionType.FungibleAsset, ...args };
+}
+export function RevokeNFTPermission(args: Omit<RevokeNFTPermission, "type">): RevokeNFTPermission {
+  return { type: PermissionType.NFT, ...args };
+}


The main draw here is improved type inference. To get a feel for how it would work without the factories, try consuming requestPermissions without using factories. You open a new object and are left without guidance on what the first step is. A brand new consumer of the API might not find it readily apparent that all the properties are defined based on what type is specified.

Might be good to brainstorm other possible DX around this.

should factories be moved to api file?

but good point actually

Since they share the same name as the interface, I think having them both be here is fine.

kaw2k · 2024-11-26T19:09:40Z

src/internal/permissions.ts

+        // txn.push(builder.add_batched_calls({
+        //   function: "0x1::object::grant_permission",
+        //   functionArguments: [
+        //     BatchArgument.new_signer(0), 


I think the types are getting messed up here due to GenericPermission. The type field of GenericPermission is string which also matches at the type level.

kaw2k · 2024-11-26T19:14:07Z

src/internal/permissions.ts

+  aptosConfig: AptosConfig;
+  primaryAccount: SingleKeyAccount;
+  subAccount: AbstractedEd25519Account;
+  permissions: RevokePermission[];


Without putting any thought into this, something feels off here. Need to dig a little.

kaw2k · 2024-11-26T19:14:51Z

src/internal/permissions.ts

+  const { aptosConfig, primaryAccount, subAccount, permissions } = args;
+
+  const aptos = new Aptos(aptosConfig);
+  const transaction = await aptos.transaction.build.batched_intents({


We need to change this to be the public key as well.

kaw2k · 2024-11-26T19:15:42Z

src/internal/permissions.ts

+  primaryAccount: SingleKeyAccount;
+  subAccount: AbstractedEd25519Account;
+}) {
+  const aptos = new Aptos(aptosConfig);


kaw2k · 2024-11-26T19:16:37Z

src/internal/permissions.ts

+
+}
+
+export interface FungibleAssetPermission {
+  type: PermissionType.FungibleAsset;
+  asset: string;


Let's discuss about the two types of fungible permissions (primary store vs fixed amount)

kaw2k and others added 4 commits November 5, 2024 16:42

Adds stubs for permission work

e5c09d3

someone fucking cooked here

23a9042

works

0696b0d

all the tests pass

b595260

chiumax requested a review from a team as a code owner November 25, 2024 22:07

chiumax assigned chiumax and kaw2k Nov 25, 2024

chiumax requested review from kaw2k and hardsetting and removed request for a team November 25, 2024 22:07

chiumax marked this pull request as draft November 25, 2024 22:08

chiumax changed the base branch from main to sub-accounts November 26, 2024 17:59

chiumax commented Nov 26, 2024

View reviewed changes

src/internal/permissions.ts

],

typeArguments: [],

});

case PermissionType.NFT: {

Copy link

Member Author

chiumax Nov 26, 2024

Choose a reason for hiding this comment

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

returning an array here breaks

chiumax commented Nov 26, 2024

View reviewed changes

organize

7e5b875

kaw2k reviewed Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permissioned Accounts #594

Permissioned Accounts #594

chiumax commented Nov 25, 2024 •

edited

Loading

chiumax Nov 26, 2024

chiumax Nov 26, 2024

kaw2k Nov 26, 2024

kaw2k Nov 26, 2024

kaw2k Nov 26, 2024

kaw2k Nov 26, 2024

kaw2k Nov 26, 2024

chiumax Nov 26, 2024

chiumax Nov 26, 2024

kaw2k Nov 26, 2024

kaw2k Nov 26, 2024

kaw2k Nov 26, 2024

kaw2k Nov 26, 2024

kaw2k Nov 26, 2024

kaw2k Nov 26, 2024

kaw2k Nov 26, 2024

Permissioned Accounts #594

Are you sure you want to change the base?

Permissioned Accounts #594

Conversation

chiumax commented Nov 25, 2024 • edited Loading

Description

Test Plan

Related Links

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiumax commented Nov 25, 2024 •

edited

Loading