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

chore: 🤖 review permissions #1413

Draft
wants to merge 1 commit into
base: alpha
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/api/procedures/addSecondaryAccountsWithAuth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export async function getAuthorization(

return {
permissions: {
// TODO: on chain just checks if is signed by primary key -> so we should check in prepareAddSecondaryKeysWithAuth if signer is the target primary key
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there is a few methods like this. Would we need to add an extra field to permissions for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean we don't need to check for the TxTags -> as the chain does not do it anyways

transactions: [TxTags.identity.AddSecondaryKeysWithAuthorization],
assets: [],
portfolios: [],
Expand Down
5 changes: 5 additions & 0 deletions src/api/procedures/createAsset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -329,21 +329,26 @@ export async function getAuthorization(
const transactions: (AssetTx | StatisticsTx)[] = [TxTags.asset.CreateAsset];

if (status === TickerReservationStatus.Free) {
// OK runs with identity perms
transactions.push(TxTags.asset.RegisterUniqueTicker);
}
if (status !== TickerReservationStatus.AssetCreated) {
// TODO: might need asset perms as it checks it with agent permissions & that checks for the asset as well
transactions.push(TxTags.asset.LinkTickerToAssetId);
}

if (documents?.length) {
// TODO: might need asset perms as it checks it with agent permissions & that checks for the asset as well
transactions.push(TxTags.asset.AddDocuments);
}

if (customTypeData?.rawId.isEmpty) {
// OK runs with identity perms
transactions.push(TxTags.asset.RegisterCustomAssetType);
}

if (initialStatistics?.length) {
// TODO: might need asset perms as it checks it with agent permissions & that checks for the asset as well
transactions.push(TxTags.statistics.SetActiveAssetStats);
}

Expand Down
1 change: 1 addition & 0 deletions src/api/procedures/createChildIdentities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ export async function getAuthorization(

return {
permissions: {
// TODO: might need just check if caller is parent of the child to be created -> Self::ensure_primary_key -> so checks primary key of the signer -> no extra perm checks performed -> so tx could be removed `pallets/identity/src/keys.rs` 506
transactions: [TxTags.identity.CreateChildIdentities],
assets: [],
portfolios: [],
Expand Down
1 change: 1 addition & 0 deletions src/api/procedures/createChildIdentity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ export async function getAuthorization(

return {
permissions: {
// TODO: might need just check if caller is parent of the child to be created -> Self::ensure_primary_key -> so checks primary key of the signer -> no extra perm checks performed -> so tx could be removed `pallets/identity/src/keys.rs` 458
transactions: [TxTags.identity.CreateChildIdentity],
assets: [],
portfolios: [],
Expand Down
5 changes: 5 additions & 0 deletions src/api/procedures/createNftCollection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -311,22 +311,27 @@ export async function getAuthorization(
}

if (status === TickerReservationStatus.Free) {
// TODO: might need asset perms as it checks it with agent permissions & that checks for the asset as well
transactions.push(TxTags.asset.RegisterUniqueTicker);
}
if (status !== TickerReservationStatus.AssetCreated) {
// TODO: might need asset perms as it checks it with agent permissions & that checks for the asset as well
transactions.push(TxTags.asset.LinkTickerToAssetId);
}

if (needsLocalMetadata) {
// TODO: might need asset perms as it checks it with agent permissions & that checks for the asset as well
transactions.push(TxTags.asset.RegisterAssetMetadataLocalType);
}

if (documents?.length) {
// TODO: might need asset perms as it checks it with agent permissions & that checks for the asset as well
transactions.push(TxTags.asset.AddDocuments);
}

const permissions = {
transactions,
// TODO: might need asset perms -> `pallets/nft/src/lib.rs` 268 "Verifies if the caller has asset permission and if the asset is an NFT."
assets: [],
portfolios: [],
};
Expand Down
1 change: 1 addition & 0 deletions src/api/procedures/issueNft.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ export function getAuthorization(
permissions: {
transactions: [TxTags.nft.IssueNft],
assets: [collection],
// TODO: might need portfolio perms "ensure_origin_asset_and_portfolio_permissions" `pallets/nft/src/lib.rs` 359
portfolios: [],
},
};
Expand Down
1 change: 1 addition & 0 deletions src/api/procedures/issueTokens.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export function getAuthorization(
permissions: {
transactions: [TxTags.asset.Issue],
assets: [asset],
// TODO: might need portfolio perms "ensure_origin_asset_and_portfolio_permissions" `pallets/asset/src/lib.rs` 1020 -> though since it is called by the signing did, should be fine
portfolios: [],
},
};
Expand Down
3 changes: 3 additions & 0 deletions src/api/procedures/modifyMultiSig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,16 +195,19 @@ export function getAuthorization(
storage: { signersToAdd, signersToRemove },
} = this;

// TODO: not sure if we need to check for tx here -> permissions checks for did of caller and if the signer is multisig admin, though might need to look how transactions listed here are checked.. there are additional checks in the pallet that check for cdd auth and if the the signer to add is not a primary key of another acc
const transactions = [];
if (signersToAdd.length > 0) {
transactions.push(TxTags.multiSig.AddMultisigSignersViaCreator);
}

if (signersToRemove.length > 0) {
// similar here
transactions.push(TxTags.multiSig.RemoveMultisigSignersViaCreator);
}

if (newRequiredSignatures) {
// same here
transactions.push(TxTags.multiSig.ChangeSigsRequiredViaCreator);
}

Expand Down
1 change: 1 addition & 0 deletions src/api/procedures/modifySignerPermissions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export async function getAuthorization(

return {
permissions: {
// TODO: while set_permission_to_signer is mentioned in benchmark data, didn't find it in pallet -> might it be `set_secondary_key_permissions` ? `pallets/identity/src/lib.rs` 499 -> if so then not sure if we need to have the tx here as it checks "ensure_primary_key" and "ensure_secondary_key" plus some other validation
transactions: [TxTags.identity.SetPermissionToSigner],
assets: [],
portfolios: [],
Expand Down
1 change: 1 addition & 0 deletions src/api/procedures/payDividends.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ export async function getAuthorization(
return {
permissions: {
transactions: [TxTags.capitalDistribution.PushBenefit],
// TODO: might need asset perms as it checks it with agent permissions & that checks for the asset as well `pallets/corporate-actions/src/distribution/mod.rs` 437
assets: [],
portfolios: [],
},
Expand Down
1 change: 1 addition & 0 deletions src/api/procedures/quitCustody.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ export function getAuthorization(
storage: { portfolioId },
} = this;
return {
// TODO: chain checks if identity can execute tx and if it is custodian -> so no reason for checking portfolio -> roles should be fine
permissions: {
transactions: [TxTags.portfolio.QuitPortfolioCustody],
assets: [],
Expand Down
1 change: 1 addition & 0 deletions src/api/procedures/removeMultiSigPayer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ export function getAuthorization(this: Procedure<Params, void, Storage>): Proced
const transactions = [];

if (isMultiSigSigner) {
// TODO: might not need these -> chain checks if signer is multisig, checks if paying did exists and then deposits event -> so prepareRemoveMultiSigPayer does the required checks
transactions.push(TxTags.multiSig.RemovePayer);
} else {
transactions.push(TxTags.multiSig.RemovePayerViaPayer);
Expand Down
1 change: 1 addition & 0 deletions src/api/procedures/removeSecondaryAccounts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export async function prepareRemoveSecondaryAccounts(
export const removeSecondaryAccounts = (): Procedure<RemoveSecondaryAccountsParams> =>
new Procedure(prepareRemoveSecondaryAccounts, {
permissions: {
// TODO: might not need this -> chain checks if called by primary key, checks if secondary key exists and can be unlinked, then unlinks
Copy link
Collaborator

Choose a reason for hiding this comment

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

chain checks if called by primary key,

I don't get this. Technically we don't need to check any permissions since the chain always checks them, but we do this to avoid having the chain catch obvious errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here -> so as I understood from the chain logic -> there are no specific extra checks for the Tx method -> if it is ok with those two conditions then the extrinsic will run

transactions: [TxTags.identity.RemoveSecondaryKeys],
assets: [],
portfolios: [],
Expand Down
2 changes: 2 additions & 0 deletions src/api/procedures/setMultiSigAdmin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,10 @@ export async function prepareSetMultiSigAdmin(
export function getAuthorization(this: Procedure<Params>, args: Params): ProcedureAuthorization {
const transactions = [];
if (args.admin) {
// TODO: not sure if we need this -> ms checks if signer is ms, checks if ms has did and then inserts -> `pallets/multisig/src/lib.rs` 426
transactions.push(TxTags.multiSig.AddAdmin);
} else {
// TODO: same here -> plus checks if called by admin
transactions.push(TxTags.multiSig.RemoveAdminViaAdmin);
}

Expand Down
1 change: 1 addition & 0 deletions src/api/procedures/transferPolyx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export async function prepareTransferPolyx(
export function getAuthorization({ memo }: TransferPolyxParams): ProcedureAuthorization {
return {
permissions: {
// TODO: might not need these the chain checks for cdd, then tries transfer
transactions: [memo ? TxTags.balances.TransferWithMemo : TxTags.balances.Transfer],
assets: [],
portfolios: [],
Expand Down
1 change: 1 addition & 0 deletions src/api/procedures/unlinkChildIdentity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ export async function getAuthorization(

return {
permissions: {
// TODO: might not need these as prepareUnlinkChildIdentity checks if signer is primary key of either the child or parent which the same as done on chain
transactions: [TxTags.identity.UnlinkChildIdentity],
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the TxTags are given in the case where as secondary key is performing the given action, and if that key has the permission for that tx or not. Maybe in a few cases it may not be needed but my guess most of them will be required

assets: [],
portfolios: [],
Expand Down
Loading