Skip to content
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

Adding method to get wallets #4

Closed
wants to merge 3 commits into from
Closed

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Sep 15, 2022

Adding method to get user wallet.

) {
this.host = this.options?.isDevelopment
? "http://localhost:3000"
: "https://api.wally.xyz";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above are formatting fixes, seems like prettier formatting in latest code is not correct.

Copy link
Contributor

@wchan2 wchan2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good generally but one question to clarify.

src/index.ts Outdated
return (await resp.json()) as Promise<SignedMessage>;
}

public async getWallets(): Promise<Wallet[]> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: What is this method going to be used for?

For a given app, I'm thinking that a single user will have only a single wallet so this may be better as getWallet instead, returning a single wallet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A: The function is going to be used to get user wallets.

I am not sure if there is a constraint about number of wallets user can have, I anyways updated the method to return first wallet returned by the api.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Does that take into account that the list of wallets (including the first wallet) are for the specific app that the token was generated for?

@mjmayank I think there was a brief mention for either one wallet per app per use or maybe a user can have multiple wallets with one app but I can't remember what the answer was.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now let's assume a user has one wallet per app

Copy link
Contributor

@wchan2 wchan2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just one question; more for precaution's sake.

@mjmayank mjmayank closed this Nov 3, 2022
@mjmayank mjmayank deleted the adding_wallets_method branch November 3, 2022 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants