-
Notifications
You must be signed in to change notification settings - Fork 523
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
Defines TypeScript types for rippled account method requests & responses #1498
Conversation
7c16a47
to
7e6b1c1
Compare
0dd7c8c
to
8549ae0
Compare
ModifiedNode: { | ||
LedgerEntryType: string | ||
LedgerIndex: string | ||
FinalFields: {[field: string]: any} |
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.
Is there any way we can narrow this from {[field: string]: any
? Does this come in the form of a LedgerEntry
that we can use to replace any
here?
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 could in theory narrow it, but as far as I understand, it's a subset of LedgerEntry
fields, and isn't necessarily a full LedgerEntry
object. I suppose this is another question for @intelliot
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 probably any (modifiable) fields in any ledger object, so I believe there are a lot of possibilities, and it'll expand in the future as more kinds of ledger objects are added in the future. I would fine with leaving this as-is, but of course, if we want to make this more specific in the future, that would be fine as well.
src/models/methods/baseMethod.ts
Outdated
|
||
export interface BaseResponse { | ||
id: number | string | ||
status: string |
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.
Can status be anything other than success
or 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.
I'm not entirely sure, which is why I left it as string
- maybe @intelliot knows
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 don't know. I think this is fine as-is, or "success" | string
if you want to document the fact that 'success' is the happy path value
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.
LGTM 👍 , given the two open questions for @intelliot are answered before merging.
…ses (#1498) * account_channels * account_currencies * account_info * account_lines * account_objects * account_offers * account_tx * gateway_balances * no ripple check * respond to comments * export methods * fix typos * respond to comments * edit BaseResponse to be more specific
…ses (#1498) * account_channels * account_currencies * account_info * account_lines * account_objects * account_offers * account_tx * gateway_balances * no ripple check * respond to comments * export methods * fix typos * respond to comments * edit BaseResponse to be more specific
…ses (#1498) * account_channels * account_currencies * account_info * account_lines * account_objects * account_offers * account_tx * gateway_balances * no ripple check * respond to comments * export methods * fix typos * respond to comments * edit BaseResponse to be more specific
High Level Overview of Change
This PR creates new TypeScript types for all rippled account methods (https://xrpl.org/account-methods.html), both the request shapes and response shapes.
Context of Change
xrpl.js v2.0 models
Type of Change
Before / After
Test Plan
This PR doesn't change any existing code, so tests are fine.