feature `NetworkID` #2216

dangell7 · 2023-02-17T04:04:04Z

High Level Overview of Change

Context of Change

Added network id to the base transaction
Added network id to the server_info response
Added network id to the binary codec
Added network id to client and modify on client.connect()
Added network id to autofill when rippled_version is 1.11.0 or later or network is hooks testnet

Type of Change

New feature (non-breaking change which adds functionality)

Test Plan

No tests were written

ckniffen · 2023-02-17T16:01:55Z

This library will already need to be updated due to the addition of 1.10.0 amendment featureXRPFees and the changes it makes to server_info. I was thinking of the autofill changes before digging into your PR description. I think that is the best way to go. It will largely allow a peaceful upgrade.

#2219

dangell7 · 2023-02-17T18:44:11Z

This library will already need to be updated due to the addition of 1.10.0 amendment featureXRPFees and the changes it makes to server_info. I was thinking of the autofill changes before digging into your PR description. I think that is the best way to go. It will largely allow a peaceful upgrade.

#2219

Yeah I agree, using the auto fill seems like the best approach.

mvadari · 2023-02-22T18:06:12Z

@dangell7 are you planning on finishing this PR yourself or are you just updating the definitions.json?

dangell7 · 2023-02-22T18:59:55Z

@dangell7 are you planning on finishing this PR yourself or are you just updating the definitions.json?

I was waiting for XRPFees because I thought it would effect this. I will finish this now though

mvadari · 2023-02-22T19:02:37Z

@dangell7 are you planning on finishing this PR yourself or are you just updating the definitions.json?

I was waiting for XRPFees because I thought it would effect this. I will finish this now though

I think they can be implemented independently (especially since this change doesn't require an amendment).

dangell7 · 2023-02-22T22:48:24Z

@mvadari I have some thoughts on this.

I added networkID to the autofill function. Technically in hooks amendment there is a fee endpoint that is different than server_info. I think the XRPFees is a prequel to that. That is why I chose to add an additional call instead of piggy back off of the server_infocall used for setFee.

However my thoughts are that the moment you are connected to the ws/rpc, you are connected with a NetworkID. The autofill adds fields to the transaction but those fields are constantly adjusting based on different inputs and the Network ID doesn't change.

So does it make sense to be in autofill or should we do a server_info call on connection of the api?

Also, is networkID a required field in the sdk? Making it optional like Fee and Sequence would allow users to use the sdk to connect to both versions of ripped. I also have that same question for the server_state model and test fixtures.

dangell7 · 2023-02-23T11:20:37Z

@mvadari I made this as a second option. The sdk doesnt emit the connected message until after the server_info response is received.

EDIT: Didn't like the second option. So I went with a 3rd option. Now its a function called after connect

Ex: await client.getNetworkID() and then in the autofill it sets the NetworkID of the client.

fixup add network id to server info fixture fixup! Revert "update tests and fixtures" This reverts commit a5deee1.

dangell7 · 2023-02-23T20:51:45Z

Regardless of the option we choose, the integration/browser tests will fail because autofill is adding a field (NetworkID) to the transaction that does not exist on the standalone or on rippled servers without the amendment enabled.

mvadari · 2023-02-23T21:46:34Z

This PR should not be merged in until the rippled PR is in develop (ideally not until it's in a new rippled release).

dangell7 · 2023-02-23T22:34:48Z

This PR should not be merged in until the rippled PR is in develop (ideally not until it's in a new rippled release).

Wouldn't it still fail for mainnet until the amendment was passed and all servers upgraded?

I think we would need to check if the amendment is enabled before adding the NetworkID to the transaction in the autofill but that sounds overkill.

mvadari · 2023-02-23T22:36:34Z

This PR should not be merged in until the rippled PR is in develop (ideally not until it's in a new rippled release).

Wouldn't it still fail for mainnet until the amendment was passed and all servers upgraded?

I think we would need to check if the amendment is enabled before adding the NetworkID to the transaction in the autofill but that sounds overkill.

There's no amendment, there's no change for mainnet. The only change is in testnets, and the only testnet that has this feature right now is the hooks one. So if we merge this in now, then it screws over everyone working on the Testnet or Devnet.

intelliot · 2023-02-23T22:42:37Z

I think you can change this PR so that when xrpl.js connects to a server that doesn't use a NetworkID (including Mainnet, Testnet, Devnet, etc) it doesn't set the NetworkID field. That way, it wouldn't break anything, and also doesn't need to wait for anything.

dangell7 · 2023-02-24T21:44:28Z

Usage: https://github.com/Transia-RnD/xls-playground/blob/main/ts/test/networkID.test.ts

client.networkID = await client.getNetworkID();

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

packages/xrpl/test/setupClient.ts

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

packages/xrpl/src/sugar/autofill.ts

mvadari

lgtm but I think @justinr1234 should review the tests before this is merged

Co-authored-by: Mayukha Vadari <[email protected]>

packages/xrpl/src/client/index.ts

packages/xrpl/src/sugar/autofill.ts

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

justinr1234 · 2023-06-07T23:25:34Z

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

 const Fee = '10'
 const Sequence = 1432
 const LastLedgerSequence = 2908734
+const HOOKS_TESTNET_ID = 21338


Is this ID set in stone and will never change?

@mvadari What do you think? I can use the faucet to retrieve the ID for hooks testnet but it would make autofill a bit slower on the first call. I think the ID for each networks should remain unchanged but I might be wrong

I don't think it changes, but if they switch to a v4 or something at some point then it might change then.
Perhaps a better solution is some sort of force-network-id flag? Or just tell people that if they're using the hooks testnet they'll have to add the network ID in themselves.

justinr1234 · 2023-06-07T23:30:35Z

packages/xrpl/test/integration/onConnect.test.ts

-          client.disconnect().then(() => resolve())
-        })
-        client.connect()
+      client.on('connected', function () {


@pdp2121 what error is caused? Can you give more information?

This leads me to believe your code needs to be enhanced to handle the case where the client is disconnected after you've already called server_info --- or that when your call to server_info is ready to be made, the client could already be disconnected.

I'm curious to know the error before we call this resolved

…nto network-id

justinr1234 · 2023-06-14T16:55:51Z

@pdp2121 can you update with main

…nto network-id

dangell7 added 2 commits February 16, 2023 23:01

update definitions

c809bd8

add network id to base transaction

b241779

dangell7 changed the title ~~Add Network ID~~ [DO NOT MERGE] feature NetworkID Feb 17, 2023

dangell7 changed the title ~~[DO NOT MERGE] feature NetworkID~~ feature NetworkID Feb 17, 2023

dangell7 changed the title ~~feature NetworkID~~ [DO NOT MERGE] feature NetworkID Feb 17, 2023

dangell7 added 5 commits February 22, 2023 17:44

Merge branch 'main' into network-id

3d06185

update server info

e36912c

update base tx

69c7058

add network id to autofill

294c1cb

update tests and fixtures

a5deee1

network id option 3

07ff763

fixup add network id to server info fixture fixup! Revert "update tests and fixtures" This reverts commit a5deee1.

mvadari mentioned this pull request Feb 23, 2023

[DO NOT MERGE] feature Hooks #2217

Closed

6 tasks

dangell7 added 3 commits February 24, 2023 16:22

update autofill

55fe97d

update getNetworkID

37c0346

remove set network func

cbd4f9f

dangell7 changed the title ~~[DO NOT MERGE] feature NetworkID~~ feature NetworkID Feb 24, 2023

dangell7 marked this pull request as ready for review February 24, 2023 21:40

pdp2121 and others added 2 commits June 5, 2023 13:19

fix lint

6e4531b

Merge branch 'main' into network-id

e85d077

pdp2121 requested a review from mvadari June 5, 2023 18:01

mvadari reviewed Jun 5, 2023

View reviewed changes

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

mvadari reviewed Jun 5, 2023

View reviewed changes

packages/xrpl/test/setupClient.ts Outdated Show resolved Hide resolved

mvadari reviewed Jun 5, 2023

View reviewed changes

packages/xrpl/test/client/autofill.test.ts Outdated Show resolved Hide resolved

pdp2121 added 2 commits June 6, 2023 12:12

fix test mock setup

a940b7c

add base tx test

a4a6306

pdp2121 requested a review from mvadari June 6, 2023 16:30

mvadari reviewed Jun 7, 2023

View reviewed changes

packages/xrpl/src/sugar/autofill.ts Show resolved Hide resolved

mvadari approved these changes Jun 7, 2023

View reviewed changes

Update packages/xrpl/src/sugar/autofill.ts

d7d6ae8

Co-authored-by: Mayukha Vadari <[email protected]>

pdp2121 requested a review from justinr1234 June 7, 2023 20:27

justinr1234 requested changes Jun 7, 2023

View reviewed changes

add try catch for server_info req

3a865eb

pdp2121 mentioned this pull request Jun 12, 2023

remove buildVersion check to autofill NetworkID #2339

Closed

pdp2121 and others added 3 commits June 12, 2023 14:17

docs fixes

2fa361c

Merge branch 'network-id' of https://github.com/Transia-RnD/xrpl.js i…

acf76f3

…nto network-id

Merge branch 'main' into network-id

2e3bc5d

pdp2121 requested a review from justinr1234 June 12, 2023 18:29

pdp2121 and others added 3 commits June 12, 2023 18:09

small logic fix for version

8c3ef3c

Merge branch 'network-id' of https://github.com/Transia-RnD/xrpl.js i…

1264d45

…nto network-id

Merge branch 'main' into network-id

fd550a5

justinr1234 approved these changes Jun 14, 2023

View reviewed changes

pdp2121 and others added 3 commits June 14, 2023 14:31

emit connected after server_info

5f50654

Merge branch 'network-id' of https://github.com/Transia-RnD/xrpl.js i…

17fcddf

…nto network-id

Merge branch 'main' into network-id

f3f5b3a

pdp2121 merged commit 5b0989d into XRPLF:main Jun 14, 2023

mvadari mentioned this pull request Aug 20, 2023

autofill doesn't cover NetworkID #2438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature `NetworkID` #2216

feature `NetworkID` #2216

dangell7 commented Feb 17, 2023 •

edited by pdp2121

Loading

ckniffen commented Feb 17, 2023 •

edited

Loading

dangell7 commented Feb 17, 2023

mvadari commented Feb 22, 2023

dangell7 commented Feb 22, 2023

mvadari commented Feb 22, 2023

dangell7 commented Feb 22, 2023 •

edited

Loading

dangell7 commented Feb 23, 2023 •

edited

Loading

dangell7 commented Feb 23, 2023 •

edited

Loading

mvadari commented Feb 23, 2023

dangell7 commented Feb 23, 2023

mvadari commented Feb 23, 2023

intelliot commented Feb 23, 2023

dangell7 commented Feb 24, 2023

mvadari left a comment

justinr1234 Jun 7, 2023

pdp2121 Jun 8, 2023

mvadari Jun 8, 2023

justinr1234 Jun 7, 2023

justinr1234 commented Jun 14, 2023

feature NetworkID #2216

feature NetworkID #2216

Conversation

dangell7 commented Feb 17, 2023 • edited by pdp2121 Loading

High Level Overview of Change

Context of Change

Type of Change

Test Plan

ckniffen commented Feb 17, 2023 • edited Loading

dangell7 commented Feb 17, 2023

mvadari commented Feb 22, 2023

dangell7 commented Feb 22, 2023

mvadari commented Feb 22, 2023

dangell7 commented Feb 22, 2023 • edited Loading

dangell7 commented Feb 23, 2023 • edited Loading

dangell7 commented Feb 23, 2023 • edited Loading

mvadari commented Feb 23, 2023

dangell7 commented Feb 23, 2023

mvadari commented Feb 23, 2023

intelliot commented Feb 23, 2023

dangell7 commented Feb 24, 2023

mvadari left a comment

Choose a reason for hiding this comment

justinr1234 Jun 7, 2023

Choose a reason for hiding this comment

pdp2121 Jun 8, 2023

Choose a reason for hiding this comment

mvadari Jun 8, 2023

Choose a reason for hiding this comment

justinr1234 Jun 7, 2023

Choose a reason for hiding this comment

justinr1234 commented Jun 14, 2023

feature `NetworkID` #2216

feature `NetworkID` #2216

dangell7 commented Feb 17, 2023 •

edited by pdp2121

Loading

ckniffen commented Feb 17, 2023 •

edited

Loading

dangell7 commented Feb 22, 2023 •

edited

Loading

dangell7 commented Feb 23, 2023 •

edited

Loading

dangell7 commented Feb 23, 2023 •

edited

Loading