-
Notifications
You must be signed in to change notification settings - Fork 524
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
Add support for Checks #853
Conversation
intelliot
commented
Feb 16, 2018
- See Checks (RIPD-1487) rippled#2245
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.
Thanks for getting to this so quickly, @intelliot. I'm, of course, reading this as a TypeScript illiterate. But it all looks reasonable to me.
I'm wondering if ripple-lib has support for finding account objects, like Offers. If it does, you might consider adding support for finding the checks that are associated with an account. There are two kinds of checks that can be associated with an account: those written by the account, and checks that are the destination of the account. Folks may want to know about either.
What I've seen here looks good to me. 👍
}, | ||
"deliverMin": { | ||
"$ref": "laxAmount", | ||
"description": "Redeem the Check for at least this amount as for as much as possible. The currency must match that of the sendMax of the corresponding CheckCreate transaction. You must provide either this field or amount." |
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.
"...amount or for as much..."?
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.
and
:)
}, | ||
"sendMax": { | ||
"$ref": "laxAmount", | ||
"description": "Amount of source currency the check is allowed to debit the sender, including transfer fees on non-XRP currencies." |
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.
Nice description.
"destinationTag": { | ||
"$ref": "tag", | ||
"description": "Destination tag that identifies the reason for the check, or a hosted recipient to pay." | ||
}, |
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'll mention is passing that a "sourceTag" is also an allowed field. But that's allowed in any transaction, so you may be handling that elsewhere.
src/ledger/parse/check-cash.ts
Outdated
// for as much as possible | ||
deliverMin: parseAmount(tx.DeliverMin).value | ||
|
||
// *must* include either Amount or DeliverMin, but not both |
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.
👍
There's |
8836f2a
to
75a280b
Compare
Today you can call |
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.
Tests are good. I don't read javascript well enough to have a useful opinion here. But I do like having tests. 😄 👍
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! Added a few comments
src/ledger/parse/check-create.ts
Outdated
import {removeUndefined} from '../../common' | ||
import parseAmount from './amount' | ||
|
||
function parseCheckCreate(tx: any): Object { |
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.
We're moving towards typing the output of these parsing functions so that they're easier to consume (also a great place to document the formatted type).
So instead of returning a type Object
you would return a type FormattedCheckCreate
, something like this:
export type FormattedCheckCreate = {
// account that can cash the check.
destination: string,
// amount the check is allowed to debit the sender, including transfer fees on non-XRP currencies.
sendMax: Amount,
// (Optional) identifies the reason for the check, or a hosted recipient.
destinationTag?: string,
// (Optional) time in seconds since the Ripple Epoch.
expiration?: string,
// (Optional) 256-bit hash representing a specific reason or identifier.
invoiceID?: string
}
function parseCheckCreate(tx: any): FormattedCheckCreate {
// ...
}
Can you do this for the other two new parsers?
src/transaction/check-cancel.ts
Outdated
|
||
function createCheckCancelTransaction(account: string, | ||
cancel: CheckCancel | ||
): Object { |
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.
Object
actually doesn't mean an arbitrary object like it did in Glow, it's closer to an empty object than an arbitrary one. Change to any
or object
(lower-case) to represent any arbitrary object, and then you should be able to remove the : any
you had to add to the txJSON below.
src/transaction/check-cash.ts
Outdated
} | ||
|
||
if (checkCash.amount && checkCash.deliverMin) { | ||
throw new ValidationError('"amount" and "deliverMin" properties on ' |
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.
We tend to do data validation as the first thing in the function. It's a good general rule so that you don't have to worry about what logic is safe to do pre-validation vs. post-validation (ex: if someone later had to add something to the txJSON definition above).
@FredKSchott Thank you! Have a second look? |
@shekenahglory @wilsonianb Would love if you guys could review as well. |
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!