Skip to content

feat(interledger): ilp account api #61

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

Merged
merged 19 commits into from
Aug 16, 2021
Merged

Conversation

cairin
Copy link
Collaborator

@cairin cairin commented Jul 27, 2021

Changes proposed in this pull request

Context

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass

}

type CreateIlpAccountMutationResponse implements MutationResponse {
code: String!
Copy link
Contributor

Choose a reason for hiding this comment

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

What does code refer to in all of these responses? Is it the string error enum?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's meant to represents the status of the data transfer, like an HTTP status code.

Copy link

@sharafian sharafian left a comment

Choose a reason for hiding this comment

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

I think all the Ints in the schema need to be changed to allow UInt64 and up. Overall this all makes sense to me

superAccount: IlpAccount
subAccounts(
"Paginating forwards: the cursor before the the requested page."
after: String

Choose a reason for hiding this comment

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

can we create an input type for Pagination so that we don't need to keep re-defining this?

Copy link
Collaborator Author

@cairin cairin Jul 29, 2021

Choose a reason for hiding this comment

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

We can, theoretically, but I don't think we should.

Firstly, the pagination algorithm we use (GraphQL Cursor Connections Specification) defines it this way. This is mostly to optimize for the clients side - making it easier to cache and fetch more data without necessarily having to program the logic each time. Of coarse this can be adapted to take in an input rather than individual arguments. Relevant apollo docs. Example implementation: Shopify pagination spec.

Secondly, it seems like standard practice to only use input objects for mutations, whereas queries usually have individual arguments. Example: Github graphql.

Lastly, a graphql input can't implement an interface. The fields on an input object type can themselves refer to input object types, but you can't mix input and output types in your schema. So you would have to redefine this anyway for each input:

type Query {
  ilpAccounts(input: IlpAccountsInput): IlpAccountsConnection!
}

input IlpAccountsInput {
  "Paginating forwards: the cursor before the the requested page."
  after: String
  "Paginating backwards: the cursor after the the requested page."
  before: String
  "Paginating forwards: The first **n** elements from the page."
  first: Int
  "Paginating backwards: The last **n** elements from the page."
  last: Int
}

Which just feels like unnecessary abstraction, and also more typing ⌨️

We could of course implement it like this:

type Query {
  ilpAccounts(pagination: PaginationInput, other: SomeOtherArguments): IlpAccountsConnection!
}

input PaginationInput {
  "Paginating forwards: the cursor before the the requested page."
  after: String
  "Paginating backwards: the cursor after the the requested page."
  before: String
  "Paginating forwards: The first **n** elements from the page."
  first: Int
  "Paginating backwards: The last **n** elements from the page."
  last: Int
}

But we'd have to agree that going against the standard is a good idea.

@github-actions github-actions bot added pkg: backend Changes in the backend package. pkg: interledger Changes in the interledger package. type: source Changes business logic labels Jul 29, 2021
@github-actions github-actions bot added the type: tests Testing related label Jul 31, 2021
"Extend Trustline"
extendTrustline(
"The id of the trustline to extend."
trustlineId: ID!
Copy link
Contributor

@wilsonianb wilsonianb Aug 2, 2021

Choose a reason for hiding this comment

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

#51

As is, trustlines don't have an id. Trustline methods just take accountId of the sub-account to which the trustline is extended.

Similarly, there's no trustline resource, so the trustline query further above isn't necessary. Trustline info is just included with (sub)account balances.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wilsonianb Cool, I have updated the schema, please check you're happy with it!

sharafian
sharafian previously approved these changes Aug 2, 2021
@sharafian
Copy link

@cairin were there any places here where you wanted input on translating the Account service API to graphQL? Or have Brandon's clarifications on the credit/debt functionality covered everything?

Copy link
Contributor

@wilsonianb wilsonianb left a comment

Choose a reason for hiding this comment

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

The commit in #69 adds accountId and subAccountId to the credit and debt mutations

@cairin cairin marked this pull request as ready for review August 12, 2021 19:23
wilsonianb
wilsonianb previously approved these changes Aug 12, 2021
Copy link
Contributor

@wilsonianb wilsonianb left a comment

Choose a reason for hiding this comment

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

👍
With the bigint serialization fix, I can pass a string as a query/mutation variable. It's received by the resolver as a bigint, and the test client receives bigint string in the response. ✔️

I'll be submitting subsequent pull requests with accounts service resolvers.

matdehaast
matdehaast previously approved these changes Aug 16, 2021
Copy link
Collaborator

@matdehaast matdehaast 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 @cairin . There are lots of TODO's in here. We just need to make sure those get captured correctly in the issues/project board

@@ -8,6 +8,8 @@ generates:
config:
defaultMapper: Partial<{T}>
resolverTypeWrapperSignature: Promise<T>
scalars:
UInt64: bigint
Copy link
Collaborator

Choose a reason for hiding this comment

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

bigint isn't an unsigned bigint. Should we keep the scalar as UInt64?

Also I can't remember but are there any values in the account spec that could be negative?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the accounts service (and tigerbeetle) only does unsigned values

@cairin cairin merged commit 8af7813 into main Aug 16, 2021
@cairin cairin deleted the cairin-ilp-accounts-graphql branch August 16, 2021 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. pkg: interledger Changes in the interledger package. type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose account API to backend via graphQL
5 participants