Implement registry proofs #244
Conversation
mrcnski
commented
Aug 9, 2021
•
mrcnski
commented
- [BREAKING] Make client.registry.getEntryLink a standalone function and non-async
- Export crypto length constants
- Implement registry proofs
- Throw if expected headers are not found
mrcnski
force-pushed
the
registry-proofs
branch
3 times, most recently
from
3ef7b15
to
0bfbb8a
Compare
mrcnski
force-pushed
the
registry-proofs
branch
from
0bfbb8a
to
27d56f5
Compare
|
I would like to improve the "Could not verify registry proof chain" error messages but not sure how to do that without the error being overly technical and confusing. Ideas are welcome. |
|
This PR is waiting for the |
See #40030
.github/workflows/npmpublish.yml
Outdated
| @@ -14,7 +14,7 @@ jobs: | |||
| - uses: actions/checkout@v2 | |||
| - uses: actions/setup-node@v1 | |||
| with: | |||
| node-version: 16 | |||
| node-version: 16.8 | |||
There was a problem hiding this comment.
is there a reason why you are defining specific minor version of nodejs ? people are updating minor versions on their machines and it is beneficial to catch any issue with new version before it's reported by broken user code
There was a problem hiding this comment.
Yeah, it was necessary because CI breaks on Node 16.9. See commit 8e00300.
There was a problem hiding this comment.
that commit doesn't say much about the nature of the issue 8e00300
There was a problem hiding this comment.
My bad, the issue is nodejs/node#40030. Looks like it might have been resolved.
src/download.ts
Outdated
| if (isSkylinkV1(inputSkylink)) { | ||
| if (inputSkylink !== dataLink) { | ||
| throw new Error("Expected returned skylink to be the same as input data link"); | ||
| } | ||
| // If input skylink is not an entry link, no proof should be present. | ||
| if (proofArray.length > 0) { | ||
| throw new Error("Expected 'skynet-proof' header to be empty for data link"); | ||
| } | ||
| // Nothing else to do for data links, there is no proof to validate. | ||
| return; | ||
| } |
There was a problem hiding this comment.
we should assert that proof header is not set for skylink v1, checking proofArray wouldn't then make sense because in the first try-catch block you already check for if(proof)
|
The CI is failing because of a bug nodejs/node#40030. However tests do pass, which can be confirmed by running |
|
|
||
| it("should throw if skynet-portal-api header is missing", async () => { | ||
| const incompleteHeaders: Record<string, unknown> = { ...headers }; | ||
| incompleteHeaders["skynet-portal-api"] = undefined; |
|
|
||
| it("should throw if skynet-skylink header is missing", async () => { | ||
| const incompleteHeaders: Record<string, unknown> = { ...headers }; | ||
| incompleteHeaders["skynet-skylink"] = undefined; |
|
@m-cat only one real comment about the breaking changes, looks good otherwise! Will press approve when that comment gets resolved. Do we have to break compat. here? I don't see why that's absolutely necessary. edit: got confused by naming, LGTM 👍 |
