Converts permissions to a serializable class #604
Conversation
| const [capabilityKeys, capabilityValues] = Object.entries(this.capabilities).reduce( | ||
| ([keys, values], [key, value]) => [keys.concat(key), values.concat(value)], | ||
| [[] as string[], [] as boolean[]], | ||
| ); | ||
| serializer.serializeStr(JSON.stringify(capabilityKeys)); | ||
| serializer.serializeStr(JSON.stringify(capabilityValues)); |
There was a problem hiding this comment.
Serializing the capabilities is a bit messy. The base type is:
capabilities: {
mutate: true,
transfer: false
}
where each property is optional. Let me know of a better way.
There was a problem hiding this comment.
What we've been doing so far, to mimic rust enums, is the following pattern:
export abstract class Permission {
static deserialize(deserializer: Deserializer) {
const type = deserializer.deserializeStr() as PermissionType;
switch (type) {
case PermissionType.FungibleAsset: return FungibleAssetPermission.load(deserializer);
...
default: throw new Error('Unsupported permission type');
}
}
}
export class FungibleAssetPermission extends Permission {
serialize(serializer: Serializer) { ... }
load(deserializer: Deserializer) { ... }
}to be honest though, this mostly makes sense / is justified only if these types are matching rust types.
Is this the case / going to be the case? Otherwise they don't need to be classes imho
src/types/permissions.ts
Outdated
| static from({ asset, balance }: { asset: AccountAddress; balance: string }): FungibleAssetPermission { | ||
| return new FungibleAssetPermission(asset, balance); | ||
| } |
There was a problem hiding this comment.
Why not just using the constructor?
There was a problem hiding this comment.
I was mirroring other classes in the codebase that have from as a static constructor.
|
These types don't mirror the rust types. The classes have helped clean up the code in a few ways, a little complications in other. Pushing another update and calling out some other changes. |
| /** | ||
| * Revoke permission types | ||
| */ | ||
| export type RevokeFungibleAssetPermission = Pick<FungibleAssetPermission, "type" | "asset">; | ||
| export type RevokeNFTPermission = Pick<NFTPermission, "type" | "assetAddress">; |
There was a problem hiding this comment.
Note: I removed the revoking types here in favor of just using the base class. Might add these back in but modified after using the new style in connect.
| @@ -36,8 +28,8 @@ export async function getPermissions<T extends PermissionType>({ | |||
| aptosConfig: AptosConfig; | |||
| primaryAccountAddress: AccountAddress; | |||
| subAccountPublicKey: Ed25519PublicKey; | |||
| filter?: T; | |||
| }): Promise<FilteredPermissions<T>> { | |||
| filter?: new (...a: any) => T; | |||
There was a problem hiding this comment.
Trying something different with the filtering. Eventually we will hit the indexer and we can move away from calling the node directly.
This takes the class itself, so for example FungibleAssetPermission and uses instanceof for filtering.
* Converts permissions to a serializable class * updates import * Converts to single object argument approach * More work * Fixes dependency cycle * Formats code
* Converts permissions to a serializable class * updates import * Converts to single object argument approach * More work * Fixes dependency cycle * Formats code
* Converts permissions to a serializable class * updates import * Converts to single object argument approach * More work * Fixes dependency cycle * Formats code
* Converts permissions to a serializable class * updates import * Converts to single object argument approach * More work * Fixes dependency cycle * Formats code
* Converts permissions to a serializable class * updates import * Converts to single object argument approach * More work * Fixes dependency cycle * Formats code
Description
There will need to be more work here, for example I didn't convert the revoke permissions to be a serializable class. Looking to get feedback.
Test Plan
Tests pass
Related Links
Base PR #596