-
Notifications
You must be signed in to change notification settings - Fork 149
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
feat(core): get account default wallet by username method updated to … #4750
base: main
Are you sure you want to change the base?
feat(core): get account default wallet by username method updated to … #4750
Conversation
let account: Account | RepositoryError | ||
|
||
if (value.match(UsernameRegex)) { | ||
account = await AccountsRepository().findByUsername(value as Username) |
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 is not necessary, in cases like this is just return AccountsRepository().findByUsername(value as Username)
|
||
export const getAccountByUsernameOrPhone = async ( | ||
value: Username | PhoneNumber, | ||
): Promise<Account> => { |
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.
please check other app layer methods, this methods should return the type and/or the application error type
if (value.match(UsernameRegex)) { | ||
account = await AccountsRepository().findByUsername(value as Username) | ||
} else { | ||
const user = await UsersRepository().findByPhone(value as PhoneNumber) |
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.
please dont use direct cast, use the checkedTo...
methods from domain layer
import { mapError } from "@/graphql/error-map" | ||
import { AccountsRepository, UsersRepository } from "@/services/mongoose" | ||
|
||
export const getAccountByUsernameOrPhone = async ( |
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 is not what I requested... this is more like getDefaultWalletByUsernameOrPhone
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.
About this do you mean just a renaming change "getAccountByUsernameOrPhone" function to "getDefaultWalletByUsernameOrPhone"?
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.
yes.. you need to query the wallets that logic must not be at graphql layer
throw mapError(account) | ||
} | ||
|
||
const account = await Accounts.getAccountByUsernameOrPhone(username) |
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.
you need to move the wallet selection too to app layer. including throw mapError(new CouldNotFindWalletFromUsernameAndCurrencyError(username))
but please dont throw just return new CouldNotFindWalletFromUsernameAndCurrencyError(username)
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.
It is not clear to me what I should do, since I based on the other validations of the same file.
I also have doubts about the CouldNotFindWalletFromUsernameAndCurrencyError class, since that was already there, and we did not make any changes.
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.
you need to move the wallet selection logic to app layer and in app layer you must not use throw
@@ -22,11 +21,7 @@ const AccountDefaultWalletQuery = GT.Field({ | |||
throw username | |||
} | |||
|
|||
const account = await AccountsRepository().findByUsername(username) | |||
if (account instanceof Error) { |
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 cant be removed... same apply to the new method
…hub.com/tiankii/blink into feat--sending-sats-via-sms-&-onboarding
Sending Sats via SMS & Onboarding
Reference: Click here