-
Notifications
You must be signed in to change notification settings - Fork 98
feat: fetch node keys from subnet certificate #776
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
Conversation
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these like the golden tickets in willy wonka?
| | [NodeId.Labeled, ArrayBuffer, HashTree] | ||
| | [NodeId.Leaf, ArrayBuffer] | ||
| | [NodeId.Pruned, ArrayBuffer]; | ||
| | [typeof NodeId.Empty] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how did this not produce a type error before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JS agent's decoding logic simply didn't cover this case. The /subnet path never decoded correctly.
At some point, the entire certificate ought to be re-written, modeled after the rust library for comprehensiveness
| const label = labelToString(tree[1]); | ||
| const sub = hashTreeToString(tree[2]); | ||
| return `label(\n label:\n${indent(label)}\n sub:\n${indent(sub)}\n)`; | ||
| if (tree[1] instanceof ArrayBuffer && tree[2] instanceof ArrayBuffer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some links to the spec as a comment would be helpful here imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire utility function doesn't seem to work and it has no tests. I only modified it in order to make it type safe with the other lookup changes
ghost
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Nice work @krpeacock 👍
| }); | ||
|
|
||
| const data = cert.lookup(encodePath(uniquePaths[index], canisterId)); | ||
| response.certificate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a leftover?
| const nodeForks = flatten_forks(nodeTree as HashTree) as HashTree[]; | ||
| nodeForks.length; | ||
|
|
||
| this.#nodeKeys = nodeForks.map(fork => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here nodeKeys is an array of public keys.
When we verify the node signature, we know the node id and would like to find the corresponding public. For this purpose, should nodeKeys be a map from node id to public key?
In the code below, I think <node_id> is fork[1], which is ArrayBuffer.
Description
Adds support for pulling subnet ID and node public keys out of a Certificate, and making them available for the
subnetpath in CanisterStatus.Involves some refactoring of certificate decoding, so we introduce
lookupResultToBufferto constrain theCert.lookupmethod to its existing return types. This means that aHashTreewith type empty will be returned asundefined, preserving existing behavior. However, thelookup_pathfunction will now returnHashTrees.This will support the upcoming certified queries feature
Fixes # (issue)
How Has This Been Tested?
new tests, including fresh certificates from application and system subnets on local and mainnet, on
dfx 0.15.0Checklist: