feat: add support for current sidechain design #2039

mvadari · 2022-07-11T21:37:11Z

High Level Overview of Change

This PR adds support for the latest version of the sidechain design.

There are 8 new transaction types:

XChainCreateBridge (creates a new cross-chain bridge)
XChainModifyBridge (modifies the properties of a cross-chain bridge)
XChainCreateClaimID (creates a new cross-chain claim ID)
XChainCommit (locks/burns the funds on the source chain)
XChainAccountCreateCommit (creates an account on the destination chain)
XChainClaim (claims the transfer funds on the destination chain)
XChainAddClaimAttestation (submits a claim attestation from a witness server)
XChainAddAccountCreateAttestation (submits an account creation attestation from a witness server)

#2059 and #2301 should be merged into this PR before this PR is merged.

Context of Change

sidechain prod release

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Before / After

Test Plan

Tests were added for all the new features. CI passes.

packages/ripple-binary-codec/src/enums/definitions.json

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

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

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

khancode

Approving since it's fine to omit docstrings for a beta release.

ckniffen · 2023-09-20T01:21:11Z

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

+export function validateXChainModifyBridge(tx: Record<string, unknown>): void {
+ validateBaseTransaction(tx)
+
+ if (tx.XChainBridge == null) {


A lot of this logic is shared between multiple transaction types. Maybe have a base xchain validation function.

This is already done with isXChainBridge. I don't think it matters that much for the two if statements that are replicated between all the transactions. I'm can change this if you object though.

Fixed in 6594c19 - the helper functions aren't going to be used in existing models in this PR because that would be a lot of work (and make this PR even more gargantuan), but the script should work for those as well.

packages/xrpl/src/models/methods/ledgerEntry.ts

ckniffen · 2023-09-20T01:24:48Z

packages/xrpl/src/models/methods/ledgerEntry.ts

@@ -152,6 +153,30 @@ export interface LedgerEntryRequest extends BaseRequest, LookupByLedgerRequest {
 * Must be the object ID of the NFToken page, as hexadecimal
 */
 nft_page?: string
+
+ bridge_account?: string


nit: document these new fields.

cc @oeggert

ckniffen · 2023-09-20T01:27:36Z

packages/xrpl/src/models/ledger/XChainOwnedCreateAccountClaimID.ts

+ * common parameters.
+ */
+ XChainCreateAccountAttestations: Array<{
+ // TODO: add docs


nit: forgotten TODO.

cc @oeggert

ckniffen · 2023-09-20T01:32:13Z

packages/xrpl/snippets/src/bridgeTransfer.ts

+ ledgersWaited = 0
+ while (ledgersWaited < MAX_LEDGERS_WAITED) {
+ await sleep(LEDGER_CLOSE_TIME)
+ const currentBalance = await issuingClient.getXrpBalance(


That error might be helpful regardless of it if is sidechain or not.

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

ckniffen · 2023-09-21T19:43:17Z

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

+import {
+ XChainModifyBridge,
+ validateXChainModifyBridge,
+} from './XChainModifyBridge'

 /**


Rolling out a tx system like the explorer could clean this up a bit. Maybe something for 4.0

packages/xrpl/.eslintrc.js

ckniffen

Approving ahead of the documentation additions.

packages/xrpl/src/utils/getXChainClaimID.ts

packages/xrpl/test/utils/getXChainClaimID.test.ts

JST5000 · 2023-09-25T21:35:40Z

packages/xrpl/src/models/ledger/XChainOwnedClaimID.ts

+ * signature.
+ */
+ XChainClaimAttestations: Array<{
+ // TODO: add docs


Is this TODO being tracked somewhere?

Yes - @oeggert has it in his backlog

JST5000 · 2023-09-25T21:38:43Z

packages/xrpl/test/models/baseTransaction.test.ts

@@ -78,7 +78,7 @@ describe('BaseTransaction', function () {
 assert.throws(
 () => validateBaseTransaction(invalidFee),
 ValidationError,
- 'BaseTransaction: invalid Fee',
+ 'Payment: invalid field Fee',


Why was this changed from BaseTransaction to Payment?

The error was changed when I refactored the validateBaseTransaction method to use the new helper functions.

JST5000

LGTM - documentation changes can be added in a separate PR

* add XChainAddAttestation tests, fix model * add XChainClaim model tests * add XChainCommit model tests, fix typo * add XChainCreateBridge model tests * add XChainCreateClaimID model tests * fix tests * add XChainModifyBridge tests * fix tests * fix more tests * fix linting errors * update to most recent version of code * remove XChainAddAttestationBatch tests * fix rebase issues * fix linter issues * add more validation tests * fix linter errors * fix tests * switch createValidateTests to JS

khancode

LGTM, just nit comments

khancode · 2023-09-26T12:58:30Z

packages/ripple-binary-codec/src/types/xchain-bridge.ts

+ if (value instanceof XChainBridge) {
+ return value
+ }
+
+ if (isXChainBridgeObject(value)) {
+ const bytes: Array<Buffer> = []
+ this.TYPE_ORDER.forEach((item) => {
+ const { name, type } = item
+ if (type === AccountID) {
+ bytes.push(Buffer.from([0x14]))
+ }
+ const object = type.from(value[name])
+ bytes.push(object.toBytes())
+ })
+ return new XChainBridge(Buffer.concat(bytes))
+ }
+
+ throw new Error('Invalid type to construct an XChainBridge')


nit: I prefer throwing validation errors near the top of a function.

Suggested change

if (value instanceof XChainBridge) {

return value

}

if (isXChainBridgeObject(value)) {

const bytes: Array<Buffer> = []

this.TYPE_ORDER.forEach((item) => {

const { name, type } = item

if (type === AccountID) {

bytes.push(Buffer.from([0x14]))

}

const object = type.from(value[name])

bytes.push(object.toBytes())

})

return new XChainBridge(Buffer.concat(bytes))

}

throw new Error('Invalid type to construct an XChainBridge')

if (value instanceof XChainBridge) {

return value

}

if (!isXChainBridgeObject(value)) {

throw new Error('Invalid type to construct an XChainBridge')

}

const bytes: Array<Buffer> = []

this.TYPE_ORDER.forEach((item) => {

const { name, type } = item

if (type === AccountID) {

bytes.push(Buffer.from([0x14]))

}

const object = type.from(value[name])

bytes.push(object.toBytes())

})

return new XChainBridge(Buffer.concat(bytes))

khancode · 2023-09-26T13:25:33Z

packages/xrpl/src/models/ledger/Bridge.ts

+ * transaction. If this isn't present, the {@link XChainAccountCreateCommit}
+ * transaction will fail. This field can only be present on XRP-XRP bridges.
+ */
+ MinAccountCreateAmount?: string


I also prefer optional fields being after required ones. Personally, it's easier to read. :)

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

mvadari changed the base branch from main to xrpl-codec-gen July 11, 2022 21:37

mvadari mentioned this pull request Jul 11, 2022

feat: add support for current sidechain design #2012

Closed

3 tasks

mvadari force-pushed the xrpl-codec-gen branch from 742b9d0 to d76003e Compare July 19, 2022 19:32

Base automatically changed from xrpl-codec-gen to main July 19, 2022 20:01

mvadari mentioned this pull request Jul 31, 2022

test: add model tests for new sidechain transactions #2059

Merged

1 task

mvadari force-pushed the sidechain-2.5 branch from 667f4ad to 691fb44 Compare July 31, 2022 14:48

mvadari mentioned this pull request Jul 31, 2022

remove: remove old sidechain design features #2060

Merged

1 task

mvadari requested review from ckniffen, JST5000 and khancode August 3, 2022 12:16

JST5000 reviewed Aug 3, 2022

View reviewed changes

packages/ripple-binary-codec/src/enums/definitions.json Show resolved Hide resolved

JST5000 reviewed Aug 3, 2022

View reviewed changes

packages/ripple-binary-codec/src/enums/definitions.json Outdated Show resolved Hide resolved

JST5000 reviewed Aug 3, 2022

View reviewed changes

packages/ripple-binary-codec/src/enums/definitions.json Show resolved Hide resolved

mvadari force-pushed the sidechain-2.5 branch from 3982787 to feb4bff Compare August 15, 2022 10:02

mvadari changed the title ~~feat: add support for current sidechain design~~ [DO NOT MERGE] feat: add support for current sidechain design Aug 16, 2022

mvadari marked this pull request as ready for review August 16, 2022 11:51

khancode requested changes Aug 19, 2022

View reviewed changes

khancode previously approved these changes Aug 30, 2022

View reviewed changes

mvadari force-pushed the sidechain-2.5 branch 2 times, most recently from a59fafb to ee927a1 Compare September 2, 2022 11:07

mvadari force-pushed the sidechain-2.5 branch from 00fcf0d to 92b1751 Compare September 14, 2022 23:25

mvadari force-pushed the sidechain-2.5 branch from 92b1751 to 169edfd Compare September 28, 2022 21:45

mvadari force-pushed the sidechain-2.5 branch from 9b09094 to 2670298 Compare November 9, 2022 03:27

mvadari force-pushed the sidechain-2.5 branch 3 times, most recently from aa4a306 to 8cb1ad2 Compare November 17, 2022 17:27

mvadari mentioned this pull request Nov 21, 2022

Add sidechain-specific features XRPLF/xrpl4j#245

Open

mvadari added 3 commits January 9, 2023 12:13

Update definitions.json

7719902

add new st types

3a93f60

add tests

0286b81

respond to comments

c2a10fc

mvadari force-pushed the sidechain-2.5 branch from 010abe8 to c2a10fc Compare September 15, 2023 15:18

Merge branch 'main' into sidechain-2.5

58db930

ckniffen reviewed Sep 20, 2023

View reviewed changes

mvadari added 3 commits September 20, 2023 18:35

more snippet fixes

84a8048

make validate functions more readable

6594c19

fix error message

21fc889

ckniffen reviewed Sep 21, 2023

View reviewed changes

packages/xrpl/src/models/transactions/common.ts Show resolved Hide resolved

ckniffen reviewed Sep 21, 2023

View reviewed changes

packages/xrpl/.eslintrc.js Outdated Show resolved Hide resolved

ckniffen approved these changes Sep 21, 2023

View reviewed changes

mvadari added 6 commits September 21, 2023 15:54

add getXChainClaimID method to parse metadata

94afb96

fix linter issues

75fb8e4

re-add linter rules

29f146f

clean up common

7b9dadf

fix getXChainClaimID test

b75c240

fix tests

7d7f198

ckniffen reviewed Sep 21, 2023

View reviewed changes

packages/xrpl/src/utils/getXChainClaimID.ts Show resolved Hide resolved

ckniffen reviewed Sep 21, 2023

View reviewed changes

packages/xrpl/test/utils/getXChainClaimID.test.ts Outdated Show resolved Hide resolved

mvadari added 2 commits September 21, 2023 17:26

return undefined for failed tx

2acf8d9

fix copy-paste error

3896e52

ckniffen approved these changes Sep 22, 2023

View reviewed changes

JST5000 reviewed Sep 25, 2023

View reviewed changes

JST5000 approved these changes Sep 25, 2023

View reviewed changes

mvadari changed the title ~~[DO NOT MERGE] feat: add support for current sidechain design~~ feat: add support for current sidechain design Sep 26, 2023

mvadari merged commit 91e7369 into main Sep 26, 2023
17 checks passed

mvadari deleted the sidechain-2.5 branch September 26, 2023 14:01

khancode approved these changes Sep 26, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for current sidechain design #2039

feat: add support for current sidechain design #2039

mvadari commented Jul 11, 2022 •

edited

Loading

khancode left a comment

ckniffen Sep 20, 2023

mvadari Sep 21, 2023

mvadari Sep 21, 2023 •

edited

Loading

ckniffen Sep 20, 2023

mvadari Sep 21, 2023

ckniffen Sep 20, 2023

mvadari Sep 21, 2023

ckniffen Sep 20, 2023

ckniffen Sep 21, 2023

ckniffen left a comment

JST5000 Sep 25, 2023

mvadari Sep 25, 2023

JST5000 Sep 25, 2023

mvadari Sep 25, 2023

JST5000 left a comment

khancode left a comment •

edited

Loading

khancode Sep 26, 2023

khancode Sep 26, 2023

feat: add support for current sidechain design #2039

feat: add support for current sidechain design #2039

Conversation

mvadari commented Jul 11, 2022 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

Before / After

Test Plan

khancode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvadari Sep 21, 2023 • 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

ckniffen left a 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

JST5000 left a comment

Choose a reason for hiding this comment

khancode left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvadari commented Jul 11, 2022 •

edited

Loading

mvadari Sep 21, 2023 •

edited

Loading

khancode left a comment •

edited

Loading