-
Notifications
You must be signed in to change notification settings - Fork 143
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): add payment status by hash query/subscription #3899
Conversation
a3cb01e
to
e94eb92
Compare
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.
Few comments about error handling and naming things
@@ -13,20 +13,28 @@ const LnInvoicePaymentStatusQuery = GT.Field({ | |||
}, | |||
resolve: async (_, args) => { | |||
const { paymentRequest } = args.input | |||
if (paymentRequest instanceof Error) throw paymentRequest | |||
if (paymentRequest 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.
Same here, I think we usually throw our errors for queries and return for mutations
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.
type payload contains error.. really not sure if was on purpose or because it is used to in subscription... do you remember?
resolve: async (_, args) => { | ||
const { paymentHash } = args.input | ||
if (paymentHash instanceof Error) { | ||
return { errors: [{ message: paymentHash.message }] } |
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 believe we usually throw for queries and return only for mutations
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, I copied the structure used in the original status query.. please check #3899 (comment)
@@ -27,6 +28,7 @@ export const queryFields = { | |||
realtimePrice: RealtimePriceQuery, | |||
btcPriceList: BtcPriceListQuery, | |||
lnInvoicePaymentStatus: LnInvoicePaymentStatusQuery, |
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 wonder if we should do something like this just so it's clearer internally?
lnInvoicePaymentStatus: LnInvoicePaymentStatusQuery, | |
lnInvoicePaymentStatus: LnInvoicePaymentStatusByPaymentRequestQuery, |
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.
Updated 0f67fbc
|
||
import { GT } from "@/graphql/index" | ||
import { mapError } from "@/graphql/error-map" | ||
import { mapAndParseErrorForGqlResponse } from "@/graphql/error-map" | ||
import LnInvoicePaymentStatusPayload from "@/graphql/public/types/payload/ln-invoice-payment-status" | ||
import LnInvoicePaymentStatusInput from "@/graphql/public/types/object/ln-invoice-payment-status-input" |
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.
Same comment here for whether we should be more specific internally
import LnInvoicePaymentStatusInput from "@/graphql/public/types/object/ln-invoice-payment-status-input" | |
import LnInvoicePaymentStatusByRequestInput from "@/graphql/public/types/object/ln-invoice-payment-status-by-request-input" |
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.
Updated 0f67fbc although name remains the same to avoid breaking changes name: "LnInvoicePaymentStatusInput"
} | ||
} | ||
|
||
type LnInvoicePaymentResolveSource = { | ||
errors?: IError[] | ||
status?: string | ||
paymentHash?: PaymentHash | ||
paymentRequest?: EncodedPaymentRequest | ||
} | ||
|
||
const LnInvoicePaymentStatusSubscription = { |
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.
Same here
const LnInvoicePaymentStatusSubscription = { | |
const LnInvoicePaymentStatusByRequestSubscription = { |
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.
Updated 0f67fbc
aa21f0a
to
ab07d54
Compare
These checks are now included in //dev:check-sdls
901beda
to
38f4a5f
Compare
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.
Looks good to me now
No description provided.