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.

Sign up for GitHub

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

Jump to bottom

feat: better expose invoices through gql api #3313

Merged
merged 19 commits into from
Oct 24, 2023
Merged

feat: better expose invoices through gql api #3313

merged 19 commits into from
Oct 24, 2023

Conversation

UncleSamtoshi
Copy link
Contributor

@UncleSamtoshi UncleSamtoshi commented Oct 6, 2023 •
edited
Loading

  • add invoiceByPaymentHash to Wallet gql object
  • add walletById to Account gql object
  • addTransactionByPaymentHash to Wallet gql object
  • addTransactionById to Wallet gql object
  • Create IInvoice gql interface
  • Remove LnAndWalletInvoice domain type and add LnInvoice to WalletInvoice

@UncleSamtoshi UncleSamtoshi force-pushed the add-ln-invoice-by-hash branch 2 times, most recently from 0508d90 to f0e41fe Compare October 6, 2023 15:26
@UncleSamtoshi UncleSamtoshi force-pushed the add-ln-invoice-by-hash branch from ec75ce4 to d9ecce8 Compare October 10, 2023 15:39
@UncleSamtoshi UncleSamtoshi marked this pull request as draft October 10, 2023 19:00
@UncleSamtoshi UncleSamtoshi force-pushed the add-ln-invoice-by-hash branch from d9ecce8 to b84dff6 Compare October 13, 2023 18:57
@github-actions github-actions bot added the core label Oct 13, 2023
@UncleSamtoshi UncleSamtoshi marked this pull request as ready for review October 17, 2023 20:24
@UncleSamtoshi UncleSamtoshi changed the title feat: add invoice by hash to wallet feat: better expose invoices through gql api Oct 17, 2023
nicolasburtey
nicolasburtey reviewed Oct 17, 2023
View reviewed changes
Copy link
Member

@nicolasburtey nicolasburtey left a comment

Choose a reason for hiding this comment

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

Question on invoice interface

core/api/dev/apollo-federation/supergraph.graphql Outdated Show resolved Hide resolved
@UncleSamtoshi UncleSamtoshi force-pushed the add-ln-invoice-by-hash branch from b7cc761 to 95f2d01 Compare October 18, 2023 18:01
vindard
vindard reviewed Oct 20, 2023
View reviewed changes
Copy link
Contributor

@vindard vindard left a comment

Choose a reason for hiding this comment

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

Small comments

core/api/src/graphql/shared/types/object/ln-invoice.ts Outdated Show resolved Hide resolved
core/api/src/domain/bitcoin/lightning/index.types.d.ts Outdated Show resolved Hide resolved
vindard
vindard reviewed Oct 20, 2023
View reviewed changes
Copy link
Contributor

@vindard vindard left a comment

Choose a reason for hiding this comment

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

Found some potential boundary violations. Maybe @bodymindarts can confirm if these comments make sense?

core/api/src/app/wallets/get-transaction-by-id.ts Outdated Show resolved Hide resolved
core/api/src/app/wallets/get-transactions-by-hash.ts Outdated Show resolved Hide resolved
core/api/src/app/wallets/get-invoice-for-wallet-by-hash.ts Outdated Show resolved Hide resolved
core/api/src/app/wallets/get-wallet-invoice-status.ts Outdated Show resolved Hide resolved
core/api/src/app/wallets/index.ts Outdated Show resolved Hide resolved
vindard
vindard reviewed Oct 20, 2023
View reviewed changes
Copy link
Contributor

@vindard vindard left a comment

Choose a reason for hiding this comment

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

Some comments on the LnInvoice graphql type and resolvers

core/api/src/app/wallets/get-transactions-by-hash.ts Outdated Show resolved Hide resolved
core/api/src/graphql/shared/types/object/ln-noamount-invoice.ts Show resolved Hide resolved
core/api/src/graphql/shared/types/object/ln-noamount-invoice.ts Show resolved Hide resolved
@UncleSamtoshi UncleSamtoshi force-pushed the add-ln-invoice-by-hash branch from 33f9638 to 4c19543 Compare October 20, 2023 21:39
vindard
vindard reviewed Oct 23, 2023
View reviewed changes
Copy link
Contributor

@vindard vindard left a comment

Choose a reason for hiding this comment

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

Left some small naming nits but otherwise looks good

core/api/src/services/ledger/index.ts Show resolved Hide resolved
core/api/src/services/mongoose/wallets.ts Show resolved Hide resolved
core/api/src/services/mongoose/wallet-invoices.ts Show resolved Hide resolved
@UncleSamtoshi UncleSamtoshi merged commit fe7f716 into main Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolasburtey nicolasburtey nicolasburtey left review comments

@vindard vindard vindard approved these changes

Assignees
No one assigned
Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@UncleSamtoshi @nicolasburtey @vindard