introduce DeliverMax alias in Payment transactions #2684

ckeshava · 2024-04-23T17:23:01Z

High Level Overview of Change

This is a sister PR of the following xrpl-py work: XRPLF/xrpl-py#684

Context of Change

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Did you update HISTORY.md?

Yes
No, this change does not impact library users

Test Plan

Unit tests have been added, but I am unable to add integration tests. I couldn't find any examples of specifying a JSON representation (not Payment, AccountSet or any SubmittableTransaction type) as an input to the submitTransaction function. This is necessary because DeliverMax is an alias at the RPC level, it is not recognised at the protocol level. Let me know if I'm missing something.

…rithm by default. This causes breakge in downstrwam de[endencies such as Account class. Account.familySeed makes use of generateSeed

packages/xrpl/src/models/transactions/payment.ts

khancode · 2024-04-29T14:40:19Z

packages/xrpl/test/models/payment.test.ts

+  it(`verifies deliver_max alias in PaymentTransactions`, function () {
+    assert.doesNotThrow(() => validatePayment(paytxn1))
+    assert.doesNotThrow(() => validate(paytxn1))
+
+    assert.doesNotThrow(() => validatePayment(paytxn2))
+    assert.doesNotThrow(() => validate(paytxn2))
+
+    assert.doesNotThrow(() => validatePayment(paytxn3))
+    assert.doesNotThrow(() => validate(paytxn3))
+
+    assert.throws(() => validatePayment(paytxn4))
+    assert.throws(() => validate(paytxn4))
+
+    assert.throws(() => validatePayment(paytxn5))
+    assert.throws(() => validate(paytxn5))
+  })


Each different test should be separated into its own it() function.

ok, I've addressed this comment in a8036ae

Co-authored-by: Omar Khan <khancodegt@gmail.com>

This reverts commit e2bc2d6, reversing changes made to 0a2ae5c.

mvadari · 2024-04-29T18:37:46Z

packages/xrpl/src/models/transactions/payment.ts

@@ -166,6 +166,24 @@ export interface PaymentMetadata extends TransactionMetadataBase {
 export function validatePayment(tx: Record<string, unknown>): void {
  validateBaseTransaction(tx)

+  if (tx.Amount == null) {


This feels like it's changing the remit of the validate functions, which should only be throwing errors and not mutating the objects.

Hmm, I agree. But I couldn't find another appropriate place to sneak in this mutation.
Perhaps I could include this change inside ripple-binary-codec. But since this is an RPC-related change and has nothing to do with protocol level serialization/de-serialization, it doesn't feel appropriate.

I'm also questioning the necessity of this change. rippled performs validation checks on the Amount and DeliverMax fields in the Payment transactions. So even if the client libraries send incorrect input, the user should get a tem... error code from rippled.

I think having such a check in autofill would be nice.

Yes you get a tem code, but it's better to fail fast. Otherwise, why even bother to have the validate functions in the first place?

I believe you are referring to this autofill :

xrpl.js/packages/xrpl/src/client/index.ts

Line 642 in 8b596a6

public async autofill<T extends SubmittableTransaction>(

It accepts only SubmittableTransaction's. JSON-string representation of certain Payment transactions, which include DeliverMax field are not serialization friendly with encode and decode JS functions.

Please refer to this branch, it has two unit tests demonstrating this issue: d262da9

If we want to insert this change in autofill, we will need to remove Type restrictions of SubmittableTransaction and perform cosmetic changes prior to the call to decode here:

xrpl.js/packages/xrpl/src/sugar/submit.ts

Line 248 in 8b596a6

let tx =

If you're using TS, it'll get caught anyways, so that's not who we're worried about. This is just for the JS users.

That's what I'm referring to on TS vs JS. TS will error if you have a DeliverMax field, JS will let you continue because it has no type checking. I think an as any is fine for this check.

A lot of folks call autofill/sign/submit on their own, so updating submit wouldn't do anything.

Okay, I have implemented this idea here: https://github.com/ckeshava/xrpl.js/pull/new/autofill-temp

But we are losing transaction model checks, as shown by the last unit test --

xrpl.js/packages/xrpl/test/client/autofill.test.ts

Line 165 in 1312556

it('Validate Payment transaction v2 API: Payment Transaction: both DeliverMax and Amount fields are absent', async function () {

Is this what you had in mind?

What I'm suggesting is that only lines 672-696 use an as any version of the object. The rest of the checks can use the normal transaction.

Okay, here is another PR: #2689

khancode · 2024-05-01T16:00:49Z

packages/xrpl/test/models/payment.test.ts

+      InvoiceID:
+        '6F1DFD1D0FE8A32E40E1F2C05CF1C15545BAB56B617F9C6C2D63A6B704BEF59B',
+      Paths: [
+        [{ currency: 'BTC', issuer: 'r9vbV3EHvXWjSkeQ6CAcYVPGeq7TuiXY2X' }],
+      ],
+      SendMax: '100000000',
+    } as any
+
+    // JSON contains only DeliverMax field
+    paytxn2 = {
+      TransactionType: 'Payment',
+      Account: 'rUn84CUYbNjRoTQ6mSW7BVJPSVJNLb1QLo',
+      DeliverMax: '1234',
+      Destination: 'rfkE1aSy9G8Upk4JssnwBxhEv5p4mn2KTy',
+      DestinationTag: 1,
+      Fee: '12',
+      Flags: 2147483648,
+      LastLedgerSequence: 65953073,
+      Sequence: 65923914,
+      SigningPubKey:
+        '02F9E33F16DF9507705EC954E3F94EB5F10D1FC4A354606DBE6297DBB1096FE654',
+      TxnSignature:
+        '3045022100E3FAE0EDEC3D6A8FF6D81BC9CF8288A61B7EEDE8071E90FF9314CB4621058D10022043545CF631706D700CEE65A1DB83EFDD185413808292D9D90F14D87D3DC2D8CB',
+      InvoiceID:
+        '6F1DFD1D0FE8A32E40E1F2C05CF1C15545BAB56B617F9C6C2D63A6B704BEF59B',
+      Paths: [
+        [{ currency: 'BTC', issuer: 'r9vbV3EHvXWjSkeQ6CAcYVPGeq7TuiXY2X' }],
+      ],
+      SendMax: '100000000',
+    } as any
+
+    // JSON contains identical DeliverMax and Amount fields
+    paytxn3 = {
+      TransactionType: 'Payment',
+      Account: 'rUn84CUYbNjRoTQ6mSW7BVJPSVJNLb1QLo',
+      DeliverMax: '1234',
+      Amount: '1234',
+      Destination: 'rfkE1aSy9G8Upk4JssnwBxhEv5p4mn2KTy',
+      DestinationTag: 1,
+      Fee: '12',
+      Flags: 2147483648,
+      LastLedgerSequence: 65953073,
+      Sequence: 65923914,
+      SigningPubKey:
+        '02F9E33F16DF9507705EC954E3F94EB5F10D1FC4A354606DBE6297DBB1096FE654',
+      TxnSignature:
+        '3045022100E3FAE0EDEC3D6A8FF6D81BC9CF8288A61B7EEDE8071E90FF9314CB4621058D10022043545CF631706D700CEE65A1DB83EFDD185413808292D9D90F14D87D3DC2D8CB',
+      InvoiceID:
+        '6F1DFD1D0FE8A32E40E1F2C05CF1C15545BAB56B617F9C6C2D63A6B704BEF59B',
+      Paths: [
+        [{ currency: 'BTC', issuer: 'r9vbV3EHvXWjSkeQ6CAcYVPGeq7TuiXY2X' }],
+      ],
+      SendMax: '100000000',
+    } as any
+
+    // JSON contains different DeliverMax and Amount fields
+    paytxn4 = {
+      TransactionType: 'Payment',
+      Account: 'rUn84CUYbNjRoTQ6mSW7BVJPSVJNLb1QLo',
+      DeliverMax: '1234',
+      Amount: '321',
+      Destination: 'rfkE1aSy9G8Upk4JssnwBxhEv5p4mn2KTy',
+      DestinationTag: 1,
+      Fee: '12',
+      Flags: 2147483648,
+      LastLedgerSequence: 65953073,
+      Sequence: 65923914,
+      SigningPubKey:
+        '02F9E33F16DF9507705EC954E3F94EB5F10D1FC4A354606DBE6297DBB1096FE654',
+      TxnSignature:
+        '3045022100E3FAE0EDEC3D6A8FF6D81BC9CF8288A61B7EEDE8071E90FF9314CB4621058D10022043545CF631706D700CEE65A1DB83EFDD185413808292D9D90F14D87D3DC2D8CB',
+      InvoiceID:
+        '6F1DFD1D0FE8A32E40E1F2C05CF1C15545BAB56B617F9C6C2D63A6B704BEF59B',
+      Paths: [
+        [{ currency: 'BTC', issuer: 'r9vbV3EHvXWjSkeQ6CAcYVPGeq7TuiXY2X' }],
+      ],
+      SendMax: '100000000',
+    } as any
+
+    // JSON does not contain either DeliverMax or Amount field
+    paytxn5 = {
+      TransactionType: 'Payment',
+      Account: 'rUn84CUYbNjRoTQ6mSW7BVJPSVJNLb1QLo',
+      Destination: 'rfkE1aSy9G8Upk4JssnwBxhEv5p4mn2KTy',
+      DestinationTag: 1,
+      Fee: '12',
+      Flags: 2147483648,
+      LastLedgerSequence: 65953073,
+      Sequence: 65923914,
+      SigningPubKey:
+        '02F9E33F16DF9507705EC954E3F94EB5F10D1FC4A354606DBE6297DBB1096FE654',
+      TxnSignature:
+        '3045022100E3FAE0EDEC3D6A8FF6D81BC9CF8288A61B7EEDE8071E90FF9314CB4621058D10022043545CF631706D700CEE65A1DB83EFDD185413808292D9D90F14D87D3DC2D8CB',
+      InvoiceID:
+        '6F1DFD1D0FE8A32E40E1F2C05CF1C15545BAB56B617F9C6C2D63A6B704BEF59B',
+      Paths: [
+        [{ currency: 'BTC', issuer: 'r9vbV3EHvXWjSkeQ6CAcYVPGeq7TuiXY2X' }],
+      ],
+      SendMax: '100000000',
+    } as any


It's more appropriate to put these objects in their corresponding tests.

fixed in 109a88d

khancode · 2024-06-27T17:15:33Z

Closing this since the other solution was merged in - #2689

ckeshava added 2 commits March 7, 2024 12:15

generateSeed() update: If no algorithm is specified, use ed25519 algo…

bf23a74

…rithm by default. This causes breakge in downstrwam de[endencies such as Account class. Account.familySeed makes use of generateSeed

introduce DeliverMax alias in Payment transactions

08d8b08

ckeshava requested review from khancode and justinr1234 April 23, 2024 17:23

khancode requested changes Apr 29, 2024

View reviewed changes

ckeshava and others added 7 commits April 29, 2024 10:07

Update packages/xrpl/src/models/transactions/payment.ts

331828d

Co-authored-by: Omar Khan <khancodegt@gmail.com>

Update packages/xrpl/src/models/transactions/payment.ts

adfd945

Co-authored-by: Omar Khan <khancodegt@gmail.com>

Update packages/xrpl/src/models/transactions/payment.ts

545f71d

Co-authored-by: Omar Khan <khancodegt@gmail.com>

address Omar's comments

0a2ae5c

Merge https://github.com/ckeshava/xrpl.js into dMax

e2bc2d6

Merge branch 'dMax' of https://github.com/ckeshava/xrpl.js into dMax

526c83d

Revert "Merge https://github.com/ckeshava/xrpl.js into dMax"

a8036ae

This reverts commit e2bc2d6, reversing changes made to 0a2ae5c.

ckeshava requested a review from khancode April 29, 2024 18:29

mvadari reviewed Apr 29, 2024

View reviewed changes

khancode reviewed May 1, 2024

View reviewed changes

address omar's comments

109a88d

ckeshava requested a review from khancode May 2, 2024 18:40

ckeshava mentioned this pull request May 3, 2024

Aliter: Implement DeliverMax alias in Payment transactions, through autofill method #2689

Merged

9 tasks

khancode closed this Jun 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce DeliverMax alias in Payment transactions #2684

introduce DeliverMax alias in Payment transactions #2684

ckeshava commented Apr 23, 2024

khancode Apr 29, 2024

ckeshava Apr 29, 2024

mvadari Apr 29, 2024

ckeshava Apr 29, 2024

mvadari Apr 29, 2024

ckeshava May 2, 2024

mvadari May 2, 2024 •

edited

Loading

mvadari May 2, 2024

mvadari May 2, 2024

ckeshava May 2, 2024

mvadari May 2, 2024

ckeshava May 3, 2024

khancode May 1, 2024

ckeshava May 2, 2024

khancode commented Jun 27, 2024

introduce DeliverMax alias in Payment transactions #2684

introduce DeliverMax alias in Payment transactions #2684

Conversation

ckeshava commented Apr 23, 2024

High Level Overview of Change

Context of Change

Type of Change

Did you update HISTORY.md?

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvadari May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khancode commented Jun 27, 2024

mvadari May 2, 2024 •

edited

Loading