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 abstract parent class to permissions #613

Open
wants to merge 27 commits into
base: sub-accounts
Choose a base branch
from

Conversation

kaw2k
Copy link
Contributor

@kaw2k kaw2k commented Jan 6, 2025

Approach

Have a single class that all permissions inherit from. This class calls toString on its sub-class so all permissions have the same structure { type: string, payload: string }. This lets us serialize them all uniformly. This adds a bit of complexity, I'll call that out via inline comments.

Motivation

The previous implementation of permissions required us to break out each permission into its own section due to serialization. For example, in connect we would need to have each permission separated:

interface Args {
	network?: Network;
	fungibleAssetPermissions?: FungibleAssetPermission[];
	gasPermission?: GasPermission;
	nftCollectionPermissions?: NFTCollectionPermission[];
	nftPermissions?: NFTPermission[];
} 

This PR adds a base class that all permissions inherit allowing us to have homogenous arrays for use in serialization. It is easier to allow for each approach now.

interface Args {
	network?: Network;
	permissions: MovePermission[]
} 

Comparison of approaches

There are pros and cons for each approach, here is what I have observed:

Homogenous Permissions:

Pros:

  • Requesting permissions has less keys to worry about, making that DX a little easier.
    Cons:
  • The implementation is a bit messier, needing helper types and more plumbing.
  • Returning a homogenous array of permissions back to the user requires them to check the class instance of each permission, which is cumbersome.

Heterogeneous Permissions:

Pros:

  • Simpler implementation in the TS SDK.
  • Ingesting permissions is easier as we don't need to type check
    Cons:
  • More cumbersome to request permissions. For example, imagine a wallet that allows users to revoke partial permissions for a dapp. The UI would need separate arrays for each permission type, vs a single array to hold all selected permissions.

With a base class, we can offer both experiences (homogenous and heterogenous) and let individual wallets decide the DX they want to provide.

Open to critiques, enhancements, or just scrapping this.

runtian-zhou and others added 27 commits December 20, 2024 13:53
* Adds stubs for permission work

* someone fucking cooked here

* works

* all the tests pass

* organize

* respond to some comments

* use parsed permission

* fix test file

* fix

* all tests pass

---------

Co-authored-by: Rasa Welcher <[email protected]>
* Converts permissions to a serializable class

* updates import

* Converts to single object argument approach

* More work

* Fixes dependency cycle

* Formats code
@kaw2k kaw2k requested a review from chiumax January 6, 2025 18:25
@kaw2k kaw2k requested a review from a team as a code owner January 6, 2025 18:25
Comment on lines +61 to +69
type ToPayload<T> = { [K in keyof T]: string } & { readonly type: string };

interface FungibleAssetPermissionProperties {
readonly asset: AccountAddress;
readonly amount: bigint | number;
}
type FungibleAssetPermissionPayload = ToPayload<FungibleAssetPermissionProperties>;

export class FungibleAssetPermission extends MovePermission implements FungibleAssetPermissionProperties {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what is going on here:

Since the overall approach is to convert all permissions into a flat string, we need to first convert all inner properties to strings. This helper type (ToPayload) ensures we have type safety during that conversion.

Each permission has _PermissionProperties which is what is stored within the class as well as _PermissionPayload which is used in (de)serialization.

Fungible asset permissions are bit unique. I wanted to allow the constructor to accept amounts as bigint OR number for ease of consumption. The class converts that to a bigint to standardize it.

Comment on lines +82 to +84
static from(args: FungibleAssetPermissionProperties): FungibleAssetPermission;
static from(args: FungibleAssetPermissionPayload): FungibleAssetPermission;
static from(args: FungibleAssetPermissionPayload | FungibleAssetPermissionProperties): FungibleAssetPermission {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overloading from allows us to ingest the deserialized string/json object where every property is a string OR the proper types as if the dapp is creating a permission. Again, this is probably overcomplicated.

const test = GasPermission.from({ amount: 2 });
console.log(typeof test.amount);

test("Serialize a vector of MovePermissions", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example test case.

@networkdm
Copy link

Hi there, I'm working on running these test cases for the sub-accounts functionality. I understand that the sub-accounts branch on the https://github.com/aptos-labs/aptos-core repository should be compatible, but I'm noticing some newer branches related to permissioned delegation.

Could you advise on the most suitable branch to use for my testing purposes? I'd be happy to consider the newer branches if they provide any advantages for running these tests, or if there are any known issues with using the sub-accounts branch.

@kaw2k
Copy link
Contributor Author

kaw2k commented Jan 7, 2025

The branch I am currently based on for aptos-core is sub-accounts-gas. I have not yet pushed the latest commits ts-sdk branch for that base, I hope to do that this afternoon. The ts-sdk branch that will have those commits is sub-accounts-gas-permissions.

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.

5 participants