Skip to content

improve code clarity for multisig subauth #26

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.

Sign up for GitHub

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

Open
traylewin wants to merge 1 commit into
base: master
Choose a base branch
from
Open

improve code clarity for multisig subauth #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
103 changes: 48 additions & 55 deletions src/plugin/eosTransaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Helpers, Models, Interfaces, Errors } from '@open-rights-exchange/chain
import { EosAccount } from './eosAccount'
import { EosChainState } from './eosChainState'
import { getPublicKeyFromSignature, sign as cryptoSign } from './eosCrypto'
import { isValidEosSignature, isValidEosPrivateKey, toEosSignature } from './helpers'
import { EOS_IO_CODE, isValidEosPrivateKey, isValidEosSignature, toEosSignature } from './helpers'
import {
EosAuthorization,
EosActionStruct,
Expand Down Expand Up @@ -42,7 +42,7 @@ export class EosTransaction implements Interfaces.Transaction {

private _signBuffer: Buffer

private _requiredAuthorizations: EosAuthorizationPerm[]
private _requiredAuthorizationsWithSubAuths: EosAuthorizationPerm[]

private _isValidated: boolean

Expand Down Expand Up @@ -251,7 +251,7 @@ export class EosTransaction implements Interfaces.Transaction {
public async validate(): Promise<void> {
this.assertHasRaw()
// this will throw an error if an account in transaction body doesn't exist on chain
this._requiredAuthorizations = await this.fetchAuthorizationsRequired()
await this.fetchAuthorizationsRequired() // updates this._authorizationsRequired
await this.assertTransactionNotExpired()
this._isValidated = true
}
Expand Down Expand Up @@ -347,7 +347,11 @@ export class EosTransaction implements Interfaces.Transaction {
return !Helpers.isNullOrEmpty(this.signatures)
}

private checkAuthSigned(auth: EosRequiredAuthorization): boolean {
/** has enough signatures to satisfy an auth
* that is, has enough signatures to meet auth.threshold weight
* checks nested subAuths needed for msig
*/
private hasSufficientSignaturesForAuth(auth: EosRequiredAuthorization): boolean {
const weights: number[] = []
auth?.keys?.forEach(keyObj => weights.push(this.hasSignatureForPublicKey(keyObj.key) ? keyObj.weight : 0))
const authAccountsToCheck = auth?.accounts?.filter(
Expand All @@ -370,10 +374,7 @@ export class EosTransaction implements Interfaces.Transaction {
/** Whether there is an attached signature for every authorization (e.g. account/permission) in all actions */
public get hasAllRequiredSignatures(): boolean {
this.assertIsValidated()
const hasAllSignatures = this._requiredAuthorizations?.every(auth =>
this.checkAuthSigned(auth?.requiredAuthorization),
)
return hasAllSignatures
return !this.missingSignatures
}

/** Throws if transaction is missing any signatures */
Expand All @@ -387,7 +388,9 @@ export class EosTransaction implements Interfaces.Transaction {
* Retuns null if no signatures are missing */
public get missingSignatures(): EosAuthorizationPerm[] {
this.assertIsValidated()
const missing = this._requiredAuthorizations?.filter(auth => !this.checkAuthSigned(auth?.requiredAuthorization))
const missing = this._requiredAuthorizationsWithSubAuths?.filter(
auth => !this.hasSufficientSignaturesForAuth(auth?.requiredAuthorization),
)
return Helpers.isNullOrEmpty(missing) ? null : missing // if no values, return null instead of empty array
}

Expand Down Expand Up @@ -424,11 +427,7 @@ export class EosTransaction implements Interfaces.Transaction {
}

public get isMultisig(): boolean {
let requiresMoreSigs = false
this._requiredAuthorizations?.forEach(auth => {
if (auth.requiredAuthorization.keys.length > 1) requiresMoreSigs = true
})
return requiresMoreSigs
return this._requiredAuthorizationsWithSubAuths?.some(auth => auth.requiredAuthorization.keys.length > 1)
}

/** Sign the transaction body with private key(s) and add to attached signatures */
Expand Down Expand Up @@ -464,62 +463,56 @@ export class EosTransaction implements Interfaces.Transaction {

/** An array of the unique set of account/permission/publicKey for all actions in transaction
* Also fetches the related accounts from the chain (to get public keys)
* Also includes nested subAuths for an auth (used for multisig) - keys array is empty at the top-level but present in subauth
* NOTE: EOS requires async fecting, thus this getter requires validate() to be called
* call fetchAuthorizationsRequired() if needed before validate() */
get requiredAuthorizations() {
this.assertIsValidated()
return this._requiredAuthorizations
return this._requiredAuthorizationsWithSubAuths
}

/** Collect unique set of account/permission for all actions in transaction
* Retrieves public keys from the chain by retrieving account(s) when needed */
public async fetchAuthorizationsRequired(): Promise<EosAuthorizationPerm[]> {
const requiredAuths = new Set<EosAuthorizationPerm>()
const actions = this._actions
if (actions) {
actions
.map(action => action.authorization)
.forEach(auths => {
auths.forEach(auth => {
const { actor: account, permission } = auth
if (permission !== Helpers.toChainEntityName('eosio.code')) {
requiredAuths.add({ account, permission })
}
public async fetchAuthorizationsRequired() {
const requiredAuthsSet = new Set<EosAuthorizationPerm>()
// collect all unique account/permission
this._actions
?.map(action => action.authorization)
.forEach(auths => {
auths
.filter(auth => auth.permission !== EOS_IO_CODE)
.forEach(auth => {
requiredAuthsSet.add({ account: auth.actor, permission: auth.permission })
})
})
}
// get the unique set of account/permissions
const requiredAuthsArray = Helpers.getUniqueValues<EosAuthorizationPerm>(Array.from(requiredAuths))
})

const requiredAuthsUniqueArray = Helpers.getUniqueValues<EosAuthorizationPerm>(Array.from(requiredAuthsSet))
// attach public keys for each account/permission - fetches accounts from chain where necessary
const uniqueRequiredAuths = await this.addAuthToPermissions(requiredAuthsArray)
const uniqueRequiredAuths = await this.addAuthToPermissions(requiredAuthsUniqueArray)
// Attach subAuth for each accountName/permission specified as Authorization.
const uniqueRequiredAuthsWithSubAuthPromises = uniqueRequiredAuths?.map(async uAuth => {
const { accounts } = uAuth?.requiredAuthorization || {}
const accountsToGetSubAuths = accounts?.filter(
account => account.permission.permission !== Helpers.toChainEntityName('eosio.code'),
const accountsToGetSubAuths = uAuth?.requiredAuthorization?.accounts?.filter(
account => account.permission.permission !== EOS_IO_CODE,
)
if (accountsToGetSubAuths?.length > 0) {
const accountsWithAuthPromises = accountsToGetSubAuths.map(async accObj => {
const { permission } = accObj
const { actor, permission: permissionName } = permission
const permToGetAuth: EosAuthorizationPerm = {
account: actor,
permission: permissionName,
}
const [subAuthPerm] = await this.addAuthToPermissions([permToGetAuth])
const { requiredAuthorization: subRequiredAuth } = subAuthPerm
if (!Helpers.isNullOrEmpty(subRequiredAuth?.accounts)) {
Errors.throwNewError('ChainJs doesnt support nested accounts permission')
}
return { ...accObj, subAuth: subRequiredAuth }
})
const accountsWithAuth = await Promise.all(accountsWithAuthPromises)
return { ...uAuth, requiredAuthorization: { ...uAuth.requiredAuthorization, accounts: accountsWithAuth } }
}
return uAuth
if (Helpers.isNullOrEmpty(accountsToGetSubAuths)) return uAuth
// if any authorization is itself an account, dig into it and get its auths (this is used for msig transactions)
const accountsWithAuthPromises = accountsToGetSubAuths.map(async accObj => {
const { permission } = accObj
const [permWithSubAuth] = await this.addAuthToPermissions([
{ account: permission.actor, permission: permission.permission },
])
const { requiredAuthorization: subRequiredAuth } = permWithSubAuth
if (!Helpers.isNullOrEmpty(subRequiredAuth?.accounts)) {
Errors.throwNewError('ChainJs doesnt support nested accounts permission')
}
return { ...accObj, subAuth: subRequiredAuth }
})
const accountsWithAuth = await Promise.all(accountsWithAuthPromises)
return { ...uAuth, requiredAuthorization: { ...uAuth.requiredAuthorization, accounts: accountsWithAuth } }
})

const uniqueRequiredAuthsWithSubAuth = await Promise.all(uniqueRequiredAuthsWithSubAuthPromises)
return uniqueRequiredAuthsWithSubAuth
this._requiredAuthorizationsWithSubAuths = uniqueRequiredAuthsWithSubAuth
Copy link
Collaborator

@dhruv-kumar-jha dhruv-kumar-jha Sep 27, 2022

Choose a reason for hiding this comment

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

Earlier this method used to return [] containing requiredAuthsWithSubAuths but right now it doesn't return anything, This might impact the service - getRequiredAuthorizationsForTransaction method

Copy link
Contributor Author

@traylewin traylewin Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch.
But service (and other code) should use the public, standard interface Transaction.requiredAuthorizations() - that's what it is there for. Please fix service to use the correct property.

}

// TODO: This code only works if the firstPublicKey is the only required Key
Expand Down
2 changes: 2 additions & 0 deletions src/plugin/helpers/generalHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { isValidEosPublicKey } from './cryptoModelHelpers'

const EOS_BASE = 31 // Base 31 allows us to leave out '.', as it's used for account scope

export const EOS_IO_CODE = Helpers.toChainEntityName('eosio.code')

/** Returns a UNIX timestamp, that is EOS base32 encoded */

/** Returns valid EOS base32, which is different than the standard JS base32 implementation */
Expand Down
4 changes: 2 additions & 2 deletions src/plugin/models/generalModels.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,11 @@ export type EosRequiredAuthorization = {
actor: EosEntityName
permission: EosEntityName
}
/** ChainJS spesific property, generated by fetchAuthorizationsRequired
/** ChainJS specific property, generated by fetchAuthorizationsRequired
* to help missingSignatures to check if signatures satisfies authRequirement of
* provided accountName/permissions.
* NOTE: Nested accountName/permissions are not supported
* (this means accountName/permissions can not contation accounts:[] in its subAuth)
* (this means that a subAuth can not itself have a subauth (i.e. accountName/permissions can not contation accounts:[])
*/
subAuth?: EosRequiredAuthorization
weight: number
Expand Down