Conversation
aybarsayan
requested review from
ChrisChinchilla,
Dudleyneedham and
rflechtner
ChrisChinchilla
left a comment
ChrisChinchilla
left a comment
There was a problem hiding this comment.
I left a few small suggestions to things I know aren't quite correct, but I think @rflechtner might have more to say.
| @@ -1,16 +1,16 @@ | |||
| /* eslint-disable prefer-const */ | |||
| import * as Kilt from '@kiltprotocol/sdk-js' | |||
| import * as Did from '@kiltprotocol/did' | |||
There was a problem hiding this comment.
I am fairly certain this doesn't exist… DID-related methods are part of the Kilt.DidHelpers. and ``Kilt.DidResolver.`.
Or is this some other way to access the methods @rflechtner ?
There was a problem hiding this comment.
Unfortunately, I couldn’t find another way to access the Did.linkedInfoFromChain function. I found apiConfig.call.did.linkedInfoFromChain(encodedKiltnerd123Details), but it doesn’t seem to work the same way and gives errors.
There was a problem hiding this comment.
we are indeed missing a function to resolve a DID document based on a w3n. This could be something worth adding to the DidResolver that Chris mentioned
code_examples/sdk_examples/src/core_features/getting_started/04_fetch_endpoints.ts
Outdated
Show resolved
Hide resolved
| } = Kilt.Did.linkedInfoFromChain(encodedKiltnerd123Details) | ||
| console.log(`My name is kiltnerd123 and this is my DID: "${uri}"`) | ||
| document: { id } | ||
| } = Did.linkedInfoFromChain(encodedKiltnerd123Details) |
There was a problem hiding this comment.
This isn't an ideal way of doing things, but it's missing from the SDK, so @rflechtner is going to add a new method and then we can update this.
There was a problem hiding this comment.
Yes, as I explained above, unfortunately (as far as I saw), it’s not included in the new versions of the SDK.
| import axios from 'axios' | ||
|
|
||
| import * as Kilt from '@kiltprotocol/sdk-js' | ||
| import { VerifiableCredential } from '@kiltprotocol/credentials/lib/cjs/V1/types' |
There was a problem hiding this comment.
| import { VerifiableCredential } from '@kiltprotocol/credentials/lib/cjs/V1/types' | |
| import { types } from '@kiltprotocol/credentials' |
and then use types.VerfiableCredential. But we may want to move that to that /types package, doesn't seem right to have to access it that way
There was a problem hiding this comment.
Its fix on: 2fb4bf2
But types declaration works partially, even tho it connects the modules in types, its also says '"@kiltprotocol/credentials"' has no exported member named 'types'. Did you mean 'Types'?ts(2724)
There was a problem hiding this comment.
ah yes it may have been Types
| @@ -1,13 +1,12 @@ | |||
| import axios from 'axios' | |||
There was a problem hiding this comment.
@aybarsayan Not introduced by you, but this may be a good time to remove the axios package and replace it with a native fetch to make the HTTP requests.
There was a problem hiding this comment.
Yes, I can change this in a new pr 😊
rflechtner
left a comment
rflechtner
left a comment
There was a problem hiding this comment.
Double-check the import {types}from "@kiltprotocol/credentials" import (may actually be Types). But looks good to me so far!
rflechtner
left a comment
rflechtner
left a comment
There was a problem hiding this comment.
Looks good to me from what I can see here, but with the build failing it's hard to see it in the context of the code snippets
| @@ -1,17 +1,15 @@ | |||
| import * as Kilt from '@kiltprotocol/sdk-js' | |||
| import { VerifiableCredential } from '@kiltprotocol/credentials/lib/cjs/V1/types' | |||
There was a problem hiding this comment.
this is not a valid import, you're probably looking for import { types } from '@kiltprotocol/credentials' & then types.VerifiableCredential
| ```bash npm2yarn | ||
| npm init -y | ||
| npm install @kiltprotocol/sdk-js ts-node typescript axios | ||
| npm install @kiltprotocol/sdk-js @kiltprotocol/did @kiltprotocol/credentials ts-node typescript axios |
There was a problem hiding this comment.
the import of the credentials package is for the types, right? That's not ideal... we should really see that these are available from the types package or even sdk-js
3 participants
fixes #3566
Changed the code for sdk v1, its a draft, and will continue according to feedbacks
How to test:
Please provide a brief step-by-step instruction.
If necessary provide information about dependencies (specific configuration, branches, database dumps, etc.)
Checklist: