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

rh0delta · 2024-05-24T18:27:50Z

update creation of functional accounts to include existing data
issue found when trying to view programs for testnet address, verifying keys list was always empty even tho config contained the keys

Description

GitHub Issue or Linear Task

How Has This Been Tested?

- update creation of functional accounts to include existing data - issue found when trying to view programs for testnet address, verifying keys list was always empty even tho config contained the keys

…yingkeys is empty on device key

…ith build

… passed to constructor

…d around

mixmix · 2024-05-28T23:36:42Z

@frankiebee review this please

package.json

frankiebee · 2024-05-29T06:36:49Z

src/signing/index.ts

+  keyring: Keyring
  signer: Signer


i think that maybe we do keyring or signer but not both. Your thoughts?

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?

I can't comment on this

i think for now we should leave this as-is until we can solidify that registering is working as expected

frankiebee · 2024-05-29T06:42:40Z

This fixes?

issue found when trying to view programs for testnet address, verifying keys list was always empty even tho config contained the keys

you seem to only have only touched the signing file i would imagine the fix would be in sec/programs/index.ts

rh0delta · 2024-05-29T14:05:04Z

This fixes?

issue found when trying to view programs for testnet address, verifying keys list was always empty even tho config contained the keys

you seem to only have only touched the signing file i would imagine the fix would be in sec/programs/index.ts

the issue wasnt with programs, it was due to the data passed to the keyring from the cli not being persisted to the instantiation of the keyring instance, and so even if the account has verifying keys, according to the Keyring instance generated there were no keys or anything. the fix is in src/keys/index.ts. signing was touched because the device keys account had an empty list of verifying keys, so we had to work some magic to use the registration keys

mixmix

🦩 🏁

Looks good. I've pointed at what looks like gotchas to me. Great work adding some tests to keys, that feels really good as a guard-rail to me

mixmix · 2024-05-30T00:40:53Z

package.json

-    "bundle:watch": "tsup --watch",
-    "bundle:node": "tsup --platform node ",
-    "bundle:browser": "tsup --platform browser",
+    "build": "tsup",


I like this rename, AND if it was in a micro-PR by itself it would be already merged 👹

I am not hard-core on this, but just want us to watch that when we do side-tweaks like this, they will get tangles up with the rest of the PR they are in and that delays them being useful.
I do this too FWIW 😆

yeah i agree with you on this, im going to cut a new smaller PR to just address this change and we can get it merged

src/index.ts

mixmix · 2024-05-30T00:47:47Z

src/index.ts

+    admin.verifyingKeys = [...vk, verifyingKey]
+    deviceKey.verifyingKeys = [verifyingKey, ...vk]


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

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

related to #365

mixmix · 2024-05-30T00:51:49Z

src/keys/index.ts

+    const masterAccountView = JSON.parse(
+      JSON.stringify(this.accounts.masterAccountView)
+    )


🌶️ at least extract this as a function cloneDeep(this.accounts.masterAccountView)
that way it's

self documenting

as this pattern spreads, we can re-use one function

if we want to improve it we only have to swap one function

Also de-serialization like this is v slow. consider import { cloneDeep } from 'lodash'
Speed probably doesn't matter here much tho

src/keys/index.ts

mixmix · 2024-05-30T01:02:00Z

src/keys/index.ts

        this.accounts.masterAccountView[childKey][k] = v
+        setTimeout(


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

mixmix · 2024-05-30T01:11:16Z

src/signing/index.ts

+  keyring: Keyring
  signer: Signer


I can't comment on this

src/signing/index.ts

tests/end-to-end.test.ts

tests/keys.test.ts

…om:entropyxyz/sdk into naynay/keyring-registration-verifying-keys

rh0delta · 2024-05-31T18:41:04Z

we need to look more into how we are doing the storage for keyrings on the cli

frankiebee

I'm going to put my stamp of approval on this for now and get it merged in so a rc can be created for cli. But lets address deeper testing in a separate pr @mixmix

Co-authored-by: mix irving <[email protected]>

Most of what you want is in here. will make more tests a priority as of next steps and PR

frankiebee

Ive commited to the branch some of mixs docs changes! This is good to go!

rh0delta and others added 11 commits January 4, 2024 01:54

[NayNay] Registration verifying keys being stripped at Keyring

ee61cd5

- update creation of functional accounts to include existing data - issue found when trying to view programs for testnet address, verifying keys list was always empty even tho config contained the keys

wip: updated signing to pull verifying key from registration if verif…

5df040a

…yingkeys is empty on device key

wip: updated scripts to use the keyword build instead of bundle

c3fa46e

wip: there were more places to remove the keyword bundle and switch w…

935b6a1

…ith build

wip: pseudo code for new keys test, spelling fixes

0706fe6

rearranging verifying keys for device key

f31c7b4

wip: new test to determine if keyring persists data from account data…

9a98979

… passed to constructor

wip: updated register to properly update verifying keys

eea8541

wip: emit test

f3e498c

deep copy so event listeners dont change the state of the keyring

78704a4

emitter: test its functionality and the accounts that are being passe…

a9f6e68

…d around

rh0delta marked this pull request as ready for review May 28, 2024 17:48

rh0delta requested review from frankiebee, mixmix and jawndiego as code owners May 28, 2024 17:48

frankiebee reviewed May 29, 2024

View reviewed changes

mixmix previously requested changes May 30, 2024

View reviewed changes

rh0delta force-pushed the naynay/keyring-registration-verifying-keys branch from 6a6bab7 to a9f6e68 Compare May 30, 2024 16:28

rh0delta and others added 9 commits May 30, 2024 15:55

wip: debugging weird keyring issue where address wasnt returned to cli

54b72b1

wip: need to go look at main debig mode got removed here

a2ee367

weird debug mode got removed

8eead88

Merge branch 'naynay/keyring-registration-verifying-keys' of github.c…

cc2a8be

…om:entropyxyz/sdk into naynay/keyring-registration-verifying-keys

wip: test admin address

380c008

fix deepCopy lol

682e56b

fix: address test

deeadb7

frankie#dont-do-that: remove type reliance for admin

8b13909

get tests to pass!

b0eaff6

rh0delta mentioned this pull request May 31, 2024

house-keeping #355

Closed

frankiebee self-requested a review May 31, 2024 18:47

frankiebee added 3 commits June 2, 2024 10:49

doc the setTimeout

29e3c45

signing: remove uncertainty about keyring

3a7cdde

doc the test a little bit better

264c04a

frankiebee previously approved these changes Jun 2, 2024

View reviewed changes

better docs yo!

bfe1c36

Co-authored-by: mix irving <[email protected]>

frankiebee dismissed their stale review via bfe1c36 June 2, 2024 21:04

frankiebee approved these changes Jun 2, 2024

View reviewed changes

frankiebee merged commit 1b7e562 into main Jun 2, 2024
1 check passed

frankiebee deleted the naynay/keyring-registration-verifying-keys branch June 2, 2024 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rh0delta commented May 24, 2024

mixmix commented May 28, 2024

frankiebee May 29, 2024

rh0delta May 29, 2024

mixmix May 30, 2024

rh0delta May 31, 2024

frankiebee commented May 29, 2024

rh0delta commented May 29, 2024

mixmix left a comment

mixmix May 30, 2024

rh0delta May 30, 2024

mixmix May 30, 2024

rh0delta May 30, 2024

frankiebee Jun 2, 2024

mixmix May 30, 2024

mixmix May 30, 2024

mixmix May 30, 2024

mixmix May 30, 2024

rh0delta commented May 31, 2024

frankiebee left a comment

frankiebee left a comment

		admin.verifyingKeys = [...vk, verifyingKey]
		deviceKey.verifyingKeys = [verifyingKey, ...vk]

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

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

Conversation

rh0delta commented May 24, 2024

Description

GitHub Issue or Linear Task

How Has This Been Tested?

mixmix commented May 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankiebee commented May 29, 2024

rh0delta commented May 29, 2024

mixmix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rh0delta commented May 31, 2024

frankiebee left a comment

Choose a reason for hiding this comment

frankiebee left a comment

Choose a reason for hiding this comment