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

[NayNay] Registration verifying keys being stripped at Keyring #353

Merged
merged 24 commits into from
Jun 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
ee61cd5
[NayNay] Registration verifying keys being stripped at Keyring
rh0delta May 24, 2024
5df040a
wip: updated signing to pull verifying key from registration if verif…
rh0delta May 28, 2024
c3fa46e
wip: updated scripts to use the keyword build instead of bundle
rh0delta May 29, 2024
935b6a1
wip: there were more places to remove the keyword bundle and switch w…
rh0delta May 29, 2024
0706fe6
wip: pseudo code for new keys test, spelling fixes
rh0delta May 29, 2024
f31c7b4
rearranging verifying keys for device key
rh0delta May 29, 2024
9a98979
wip: new test to determine if keyring persists data from account data…
rh0delta May 29, 2024
eea8541
wip: updated register to properly update verifying keys
rh0delta May 29, 2024
f3e498c
wip: emit test
frankiebee May 29, 2024
78704a4
deep copy so event listeners dont change the state of the keyring
frankiebee May 29, 2024
a9f6e68
emitter: test its functionality and the accounts that are being passe…
frankiebee May 29, 2024
54b72b1
wip: debugging weird keyring issue where address wasnt returned to cli
rh0delta May 30, 2024
a2ee367
wip: need to go look at main debig mode got removed here
frankiebee May 30, 2024
8eead88
weird debug mode got removed
rh0delta May 30, 2024
cc2a8be
Merge branch 'naynay/keyring-registration-verifying-keys' of github.c…
frankiebee May 30, 2024
380c008
wip: test admin address
frankiebee May 30, 2024
682e56b
fix deepCopy lol
frankiebee May 30, 2024
deeadb7
fix: address test
frankiebee May 30, 2024
8b13909
frankie#dont-do-that: remove type reliance for admin
frankiebee May 30, 2024
b0eaff6
get tests to pass!
frankiebee May 30, 2024
29e3c45
doc the setTimeout
frankiebee Jun 2, 2024
3a7cdde
signing: remove uncertainty about keyring
frankiebee Jun 2, 2024
264c04a
doc the test a little bit better
frankiebee Jun 2, 2024
bfe1c36
better docs yo!
frankiebee Jun 2, 2024
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 package.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
"build:watch": "tsup --watch",
"build:node": "tsup --platform node ",
"build:browser": "tsup --platform browser",
"build:link": "yarn build && yarn unlink && yarn link",
"find-deadcode": "ts-prune",
"generate:types": "dev/bin/generate-types.sh",
"prepare": "husky",
Expand Down
15 changes: 6 additions & 9 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export default class Entropy {
signer: this.keyring.getLazyLoadAccountProxy(ChildKey.registration),
})
this.signingManager = new SignatureRequestManager({
keyring: this.keyring,
signer: this.keyring.getLazyLoadAccountProxy(ChildKey.deviceKey),
substrate: this.substrate,
adapters: opts.adapters,
Expand Down Expand Up @@ -130,15 +131,11 @@ export default class Entropy {

const verifyingKey = await this.registrationManager.register(params)
// fuck frankie TODO: Make legit function
const vk = this.keyring.accounts[ChildKey.registration].verifyingKeys || []
this.keyring.accounts[ChildKey.registration].verifyingKeys = [
...vk,
verifyingKey,
]
this.keyring.accounts[ChildKey.deviceKey].verifyingKeys = [
...vk,
verifyingKey,
]
const admin = this.keyring.getLazyLoadAccountProxy(ChildKey.registration)
const vk = admin.verifyingKeys || []
// HACK: these assignments trigger important `account-update` flows via the Proxy
admin.verifyingKeys = [...vk, verifyingKey]
deviceKey.verifyingKeys = [verifyingKey, ...vk]
frankiebee marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +137 to +138
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: you changed the order of the verifyingKeys is that significant?
from : [verifyingKeys, ...vk]
to : [...vk, verifyingKeys]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the order change is to ensure the new verifying key is the first verifying key in the list on the device keys account. this will be the new verifying key used for signing

Copy link
Collaborator

Choose a reason for hiding this comment

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

related to #365

return verifyingKey
}

Expand Down
146 changes: 83 additions & 63 deletions src/keys/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import EventEmitter from 'node:events'
import { deepCopy } from '../utils/housekeeping'
import * as utils from './utils'
import { EntropyAccount, KeyMaterial, PairMaterial } from './types/json'
import {
Expand Down Expand Up @@ -33,6 +34,7 @@ export default class Keyring {
*/

constructor (account: KeyMaterial) {
// this repersents keys that are used by the user
this.#used = ['admin', ChildKey.registration]
Object.keys(account).forEach((key) => {
if (typeof account[key] === 'object' && account[key].userContext) {
Expand All @@ -51,49 +53,47 @@ export default class Keyring {
}
const accountsJson = this.#formatAccounts(account)
this.accounts = this.#createFunctionalAccounts(accountsJson)
this.accounts.masterAccountView = accountsJson
}

/**
* Retrieves the current account information.
*
* @returns An object containing the Entropy account details.
*/
#formatAccounts (accounts: EntropyAccount): EntropyAccount {

// IMPORTANT!! WE SHOULD DECIDE IF WE WILL ALWAYS BE GENERATING UUID FOR ACCOUNTS OR IF WE
// WILL ALLOW USERS TO PASS THEIR OWN STRINGS
const { seed, type, admin } = accounts
const debug = true
const entropyAccountsJson = {
debug,
// previously was seed ? seed : utils.seedFromMnemonic(mnemonic) but this has already been derived in the constructor
// @frankie correct if im wrong here
seed: this.#seed,
type,
admin,
}

getAccount (): EntropyAccount {
const { debug, seed, type, verifyingKeys } = this.accounts.masterAccountView
const entropyAccount: EntropyAccount = { debug, seed, type, verifyingKeys }
this.#used.forEach((accountName) => {
entropyAccount[accountName] = this.accounts.masterAccountView[accountName]
})
entropyAccount.admin = this.accounts.registration
return entropyAccount
}

#createFunctionalAccounts (
masterAccountView: EntropyAccount
): AccountsEmitter {
const accounts = new EventEmitter() as AccountsEmitter
accounts.type = accounts.type || EntropyAccountType.MIXED_ACCOUNT
Object.keys(masterAccountView).forEach((name) => {
if (name) {
if (typeof masterAccountView[name] !== 'object') return
const { seed, path } = masterAccountView[name]
if (!seed) return
const { pair, address } = utils.generateKeyPairFromSeed(seed, path)
const functionalAccount = {
seed,
path,
address,
pair,
Object.keys(accounts)
.concat(ACCOUNTS)
.forEach((key) => {

let account: PairMaterial
if (entropyAccountsJson[key]) return
if (key === ChildKey.registration && admin?.seed) {
if (accounts[key]) {
account = { ...admin, ...accounts[key] }
} else {
account = admin
}

entropyAccountsJson[key] = this.#jsonAccountCreator(account, debug)
return
}
accounts[name] = functionalAccount
}
})
accounts.masterAccountView = masterAccountView
return accounts
if (accounts[key] && accounts[key].userContext) account = accounts[key]
else if (ChildKey[key]) account = { type: ChildKey[key], seed }
if (!account) return
entropyAccountsJson[key] = this.#jsonAccountCreator(account, debug)
})
if (!admin) entropyAccountsJson.admin = entropyAccountsJson[ChildKey.registration]

return entropyAccountsJson as EntropyAccount
}

/**
Expand Down Expand Up @@ -129,34 +129,50 @@ export default class Keyring {
return jsonAccount
}

#formatAccounts (accounts: EntropyAccount): EntropyAccount {
const { seed, mnemonic, type, admin } = accounts
// this is because stuff is broken outside of debug mode so making it true always
const debug = true
const entropyAccountsJson = {
debug,
seed: seed ? seed : utils.seedFromMnemonic(mnemonic),
type,
admin,
}
#createFunctionalAccounts (
masterAccountView: EntropyAccount
): AccountsEmitter {

Object.keys(accounts)
.concat(ACCOUNTS)
.forEach((key) => {
let account: PairMaterial
if (entropyAccountsJson[key]) return
if (key === ChildKey.registration && admin?.seed) {
account = admin
entropyAccountsJson[key] = this.#jsonAccountCreator(account, debug)
return
const accounts = new EventEmitter() as AccountsEmitter
accounts.type = accounts.type || EntropyAccountType.MIXED_ACCOUNT
Object.keys(masterAccountView).forEach((name) => {
if (name) {
if (typeof masterAccountView[name] !== 'object') return
const { seed, path, ...accountData } = masterAccountView[name]
if (!seed) return
const { pair, address } = utils.generateKeyPairFromSeed(seed, path)
const functionalAccount = {
...accountData,
seed,
path,
address,
pair,
}
if (accounts[key] && accounts[key].userContext) account = accounts[key]
else if (ChildKey[key]) account = { type: ChildKey[key], seed }
if (!account) return
entropyAccountsJson[key] = this.#jsonAccountCreator(account, debug)
})
accounts[name] = functionalAccount
}
})
accounts.masterAccountView = masterAccountView
return accounts
}

return entropyAccountsJson as EntropyAccount
/**
* Retrieves the current account information.
*
* @returns An object containing the Entropy account details.
*/

// IMPORTANT!! WE SHOULD DECIDE IF WE WILL ALWAYS BE GENERATING UUID FOR ACCOUNTS OR IF WE
// WILL ALLOW USERS TO PASS THEIR OWN STRINGS

getAccount (): EntropyAccount {
const { debug, seed, type, verifyingKeys } = this.accounts.masterAccountView
const entropyAccount: EntropyAccount = { debug, seed, type, verifyingKeys }
this.#used.forEach((accountName) => {
if (this.accounts[accountName] === undefined) return
entropyAccount[accountName] = deepCopy(this.accounts[accountName] as PairMaterial)
})
entropyAccount.admin = entropyAccount.registration
return entropyAccount
}

/**
Expand All @@ -178,8 +194,12 @@ export default class Keyring {
this.#used.push(childKey)
}
this.accounts[childKey][k] = v
this.accounts.emit(`#account-update`, this.getAccount())
this.accounts.masterAccountView[childKey][k] = v
// DO NOT REMOVE setTimeout UNLESS YOU SOLVED THE RACE CONDITION
setTimeout(
Copy link
Collaborator

Choose a reason for hiding this comment

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

comment why this is necessary/ needed plz
(I know, but I will also forget!)

() => this.accounts.emit(`account-update`, this.getAccount()),
10
)
return v
},
})
Expand Down
2 changes: 2 additions & 0 deletions src/keys/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ export const keysCryptoWaitReady = cryptoWaitReady()
*/

export function seedFromMnemonic (m) {
// If this is returned as a Uint8Array would it be beneficial to convert back to a string and just use strings everywhere?
// it will reduce the use of force casting to a specific type
return mnemonicToMiniSecret(m)
}

Expand Down
22 changes: 18 additions & 4 deletions src/signing/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ import { EncMsg, ValidatorInfo } from '../types/internal'
import { stripHexPrefix, sendHttpPost, toHex } from '../utils'
import { crypto } from '../utils/crypto'
import { CryptoLib } from '../utils/crypto/types'
import Keyring from '../keys'

export interface Config {
keyring: Keyring
signer: Signer
substrate: ApiPromise
adapters: { [key: string | number]: Adapter }
Expand Down Expand Up @@ -51,6 +53,7 @@ export interface UserSignatureRequest {
* Constructs a SignatureRequestManager instance.
*
* @param {Config} config - The configuration for the SignatureRequestManager.
* @param {Keyring} config.keyring - The full keyring
* @param {Signer} config.signer - The Signer instance.
* @param {ApiPromise} config.substrate - The Substrate API instance.
* @param {Adapter} config.adapters - The adapters for handling different types of transactions.
Expand All @@ -60,20 +63,23 @@ export interface UserSignatureRequest {
export default class SignatureRequestManager {
adapters: { [key: string | number]: Adapter }
crypto: CryptoLib
keyring: Keyring
signer: Signer
Comment on lines +66 to 67
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 that maybe we do keyring or signer but not both. Your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the signer in this case is coming from the deviceKeys account, we could remove the keyring from here and include the deviceKey signer and the registration signer and just switch between the two, thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't comment on this

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 think for now we should leave this as-is until we can solidify that registering is working as expected

substrate: ApiPromise

/**
* Initializes a new instance of `SignatureRequestManager`.
*
* @param {Config} config - Configuration settings for the manager.
* @param {Keyring} config.keyring - The full keyring
* @param {Signer} config.signer - The signer for authorizing transactions.
* @param {ApiPromise} config.substrate - Instance of the Polkadot/Substrate API.
* @param {Adapter[]} config.adapters - Set of adapters for handling different types of transactions.
* @param {CryptoLib} config.crypto - Instance of CryptoLib for cryptographic operations.
*/

constructor ({ signer, substrate, adapters, crypto }: Config) {
constructor ({ keyring, signer, substrate, adapters, crypto }: Config) {
this.keyring = keyring
this.signer = signer
this.substrate = substrate
this.crypto = crypto
Expand All @@ -90,7 +96,12 @@ export default class SignatureRequestManager {
*/

get verifyingKey () {
return this.signer.verifyingKeys ? this.signer.verifyingKeys[0] : undefined
let key = this.signer?.verifyingKeys?.[0]
// Returning verifying key from regsitration account if device key keys do not exist
if (!key) {
key = this.keyring.accounts.registration.verifyingKeys[0]
}
frankiebee marked this conversation as resolved.
Show resolved Hide resolved
return key
}

/*
Expand Down Expand Up @@ -137,6 +148,7 @@ export default class SignatureRequestManager {
return adapter.preSign(this.signer, msg)
})
)

// [AuxData[], ...]
const auxiliaryDataCollection = results.map(({ auxilary_data }) => {
return auxilary_data
Expand Down Expand Up @@ -247,8 +259,8 @@ export default class SignatureRequestManager {

// TODO: auxilaryData full implementation
if (auxiliaryData) {
txRequestData.auxilary_data = auxiliaryData.map((signleAuxData) =>
toHex(JSON.stringify(signleAuxData))
txRequestData.auxilary_data = auxiliaryData.map((singleAuxData) =>
toHex(JSON.stringify(singleAuxData))
)
}
// TODO handle array here
Expand Down Expand Up @@ -302,10 +314,12 @@ export default class SignatureRequestManager {
txReq.map(async (message: EncMsg) => {
// Extract the required fields from parsedMsg
const parsedMsg = JSON.parse(message.msg)

const payload = {
...parsedMsg,
msg: parsedMsg.msg,
}

const sigProof = (await sendHttpPost(
`http://${message.url}/user/sign_tx`,
JSON.stringify(payload)
Expand Down
13 changes: 13 additions & 0 deletions src/utils/housekeeping/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@




// maybe lodash maybe not?
// keep in mind registering can take over 20 seconds
// if we

/**
* returns a deep copy of the object */
export function deepCopy (thing: any): any {
return JSON.parse(JSON.stringify(thing))
}
36 changes: 34 additions & 2 deletions tests/end-to-end.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ test('End To End', async (t) => {

await run('wasm', wasmGlobalsReady())

let store = {}
const keyring = new Keyring({ seed: charlieStashSeed, debug: true })
let store = keyring.getAccount()
t.equal(store.admin.address, keyring.accounts.registration.pair.address, 'admin account should have an address and for now it should match registrations address')
keyring.accounts.on('account-update', (fullAccount) => {
store = fullAccount
})
Expand Down Expand Up @@ -69,12 +70,43 @@ test('End To End', async (t) => {
// t.equal(typeof pointer, 'string', 'valid pointer')

// register
const verifyingKeyFromRegistration = await run('register', entropy.register())
let verifyingKeyFromRegistration
const emitterTest = run(
'keyring.accounts.once#account-update',
new Promise((res, reject) => {
// TODO remove event listener if it hangs
keyring.accounts.on('account-update', (fullAccountView) => {
if (!verifyingKeyFromRegistration) return
if (!fullAccountView.admin) reject('no admin account')
if (!fullAccountView.registration) reject('no registration account')
if (!fullAccountView.deviceKey) reject('no deviceKey account')
if (!fullAccountView.registration.verifyingKeys)
reject('no registration.verifyingKeys ')
if (!fullAccountView.registration.verifyingKeys[0])
reject(
'no registration.verifyingKeys[0] this means their were no keys added'
)
const last = fullAccountView.registration.verifyingKeys.pop()
if (!last === verifyingKeyFromRegistration)
reject('verifyingKey returned in registration does not match')
if (!last === fullAccountView.deviceKey.verifyingKeys[0])
reject(
'verifyingKey on registration does not match device keys verifyingKeys[0]'
)
return res(fullAccountView)
})
})
)
verifyingKeyFromRegistration = await run('register', entropy.register())

t.equal(
verifyingKeyFromRegistration,
entropy.keyring.accounts.registration.verifyingKeys[0],
'verifyingKeys match after registration'
)
// tests the once function as well as what is
// considered to be necessary changes to the full account after emitting
await emitterTest
frankiebee marked this conversation as resolved.
Show resolved Hide resolved

//
// sign some data
Expand Down
Loading