-
Notifications
You must be signed in to change notification settings - Fork 0
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
Create wallet #2
Conversation
src/index.ts
Outdated
tags, | ||
}: CreateWalletRequest): Promise<CreateWalletResponse> { | ||
return this.requestPost( | ||
"users/create-wallet", |
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.
doesn't every method in the SDK need to send the clientID up to the API? so that it knows which app to associate the action with?
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.
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.
Where does the user pass appID? Maybe I’m missing it. Does it happen automatically?
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.
Oh I missed it here in SDK, I added appId field to api. But I would prefer to replace it with clientId or userToken.
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.
Yeah let’s do clientID since that’s how we plan to identify the app as I understand
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.
I updated the PR
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.
We should be able to re-use the /wallets
endpoint or move it to /users/wallets
to create the wallet to have to be RESTful. If we re-use the /wallets
endpoint, that would mean that we need another guard, changing up how we generate the JWT. I would have put the users id in the URI as well but it doesn't make too much sense to have a user id in the URI for the client auth SDK.
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.
I updated the api to /users/wallets
Adding userId to URI does not looks useful to me.
src/index.ts
Outdated
tags, | ||
}: CreateWalletRequest): Promise<CreateWalletResponse> { | ||
return this.requestPost( | ||
"users/create-wallet", |
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.
We should be able to re-use the /wallets
endpoint or move it to /users/wallets
to create the wallet to have to be RESTful. If we re-use the /wallets
endpoint, that would mean that we need another guard, changing up how we generate the JWT. I would have put the users id in the URI as well but it doesn't make too much sense to have a user id in the URI for the client auth SDK.
Adding function to create wallet.