more clean up for min/max adjustments

[FredKSchott]

Builds on top of #870, should be okay to merge after that PR is merged in. #870 merged in, this is ready to review/merge.

Uses some TS magic to make type checking easier, and also consolidates some destination/source amount logic to reduce repeated code.

[emschwartz]

Nitpick: why is this on a new line?

[FredKSchott]

exceeds 80 character max, enforced by our linting config :/

[emschwartz]

Ah, I see. Looks strange to me cause I'm used to the "standard" style

[emschwartz]

I think this should be an interface rather than a type

[FredKSchott]

+1, I can change that here

[intelliot]

This looks right. To understand what's going on, I read the User-Defined Type Guards section from the TypeScript docs:

http://www.typescriptlang.org/docs/handbook/advanced-types.html

[sublimator]

Worth considering if the test suite should be converted to TypeScript as well?

To handle cases like the following where accidentally specifying multiple properties:

const source = {
  address: fromAddress,
  maxAmount: {value: amount, currency: "XRP"},
  amount: {value: amount, currency: "XRP"}
};
const destination = {
  address: toAddress,
  amount: {value: amount, currency: "XRP"},
};

You can use optional fields, specified as undefined:

export interface Adjustment {
    address: string;
    amount: Amount;
    maxAmount?: undefined;
    minAmount?: undefined;
    tag?: number;
};
export interface MaxAdjustment  {
    address: string;
    amount?: undefined;
    maxAmount: Amount;
    tag?: number;
};
export interface MinAdjustment {
    address: string;
    amount?: undefined;
    minAmount: Amount;
    tag?: number;
};

Which yields:

      Type '{ address: string; amount: { value: string; currency: string; }; maxAmount: { value: string; curr...' is not assignable to type 'MaxAdjustment'.
        Types of property 'amount' are incompatible.
          Type '{ value: string; currency: string; }' is not assignable to type 'undefined'.

See also: now recommended that you just use "as foo" for consistency

[sublimator]

You can also do:

export declare type Payment = {
    paths?: string;
    memos?: Array<Memo>;
    invoiceID?: string;
    allowPartialPayment?: boolean;
    noDirectRipple?: boolean;
    limitQuality?: boolean;
} & ({
  source: Adjustment;
  destination: MinAdjustment;
} | {
  source: MaxAdjustment;
  destination: Adjustment;
} | {
    source: Adjustment,
    destination: Adjustment
});

Though the error messages are gonna be for C++ fans :)

[sublimator]

Prior comment regarding: https://github.com/ripple/ripple-lib/blob/develop/src/transaction/payment.ts#L77-L81

[sublimator]

You could even use the typescript compiler programmatically, and add test cases of strings which should work or fail ...

[intelliot]

How does a3ae796 look?

[sublimator]

Beginnings of TypeScript tests to build upon: sublimator@28e172a

[FredKSchott]

+1 for converting the tests to TS as well (in a separate PR)

---

@intelliot 👍 looks good!

---

@sublimator judging by the code, it sounds like {source: Adjustment, destination: Adjustment} isn't actually valid either, which would simplify your example above 👌

If this function was user-facing I think your more complex but most strict type definition would be worth the gross error messages. But since it wouldn't simplify the createPaymentTransaction() implementation (I don't think?) and the user never actually interacts with this type directly, I'm inclined to value the more simple definition.

[sublimator]

it sounds like {source: Adjustment, destination: Adjustment} isn't actually valid either, which would simplify your example above 👌

@FredKSchott

I never use this library, just happened to be chatting to @intelliot on Google chat recently and this issue came up. This lib gives me headaches having to mentally map back and forth between ripple terms. But why wouldn't it be a valid combination? Are you just going by the error message I linked?

What about: https://github.com/ripple/ripple-lib/blob/develop/src/transaction/payment.ts#L98-L100

?

the user never actually interacts with this type directly
this type = Payment ?

api.preparePayment ?

Though the error messages are gonna be for C++ fans :)

^^^
lol, I don't think I was seriously suggesting it :)

[FredKSchott]

api.preparePayment

Whoops, of course! Okay, well then I could definitely see the more strict type being useful, if it could catch bad user input before runtime.

Why wouldn't it be a valid combination?

I was going by this validation check in the source code itself: https://github.com/ripple/ripple-lib/blob/develop/src/transaction/payment.ts#L91, which throws a ValidationError if that's encountered.

This lib gives me headaches having to mentally map back and forth between ripple terms.

Agreed! The work we've been doing since way back in November has been to move us towards a world where ripple-lib uses the exact same concepts/terms/types/commands you're already using with rippled.