add xchain-haven #588

MartyHav · 2022-05-24T11:18:18Z

This PR adds xchain-haven package.

a few marks:

xchain-util is included as a local package as the npm version, doesn't include enum Chain.Haven yet
https://github.com/haven-protocol-org/xchainjs-lib/blob/261e0371771a0331fd1489d62f872b0d0f90e90a/packages/xchain-haven/package.json#L37
mnemonicConverter is included as a GitHub package and set to latest commit. It exposes two functions, where you can either convert a bip39 phrase into a haven or monero mnemonic. It was planed to move this package/functionality to xchain-crypto package and I would like to leave this to xchainjs maintainers.
https://github.com/haven-protocol-org/xchainjs-lib/blob/261e0371771a0331fd1489d62f872b0d0f90e90a/packages/xchain-haven/package.json#L40
haven-core-js the underlying wallet library/sdk is included as a GitHub package as well for now. I'll create a npm package of it shortly.

Besides of these points, this PR is ready to be reviewed.

veado · 2022-05-26T16:23:09Z

Thx for your PR!

* xchain-util is included as a local package as the npm version, doesn't include enum Chain.Haven yet
  https://github.com/haven-protocol-org/xchainjs-lib/blob/261e0371771a0331fd1489d62f872b0d0f90e90a/packages/xchain-haven/package.json#L37

Pls ~~add Chain.Haven to xchain-util~~ (ah, you already have done it), bump xchain-util to a next patch version + add its new version to xchain-haven. To test it locally in your xchain-haven, just run yarn build in xchain-util once.

* mnemonicConverter is included as a GitHub package and set to latest commit. It exposes two functions, where you can either convert a bip39 phrase into a haven or monero mnemonic. It was planed to move this package/functionality to xchain-crypto package and I would like to leave this to xchainjs maintainers.
  https://github.com/haven-protocol-org/xchainjs-lib/blob/261e0371771a0331fd1489d62f872b0d0f90e90a/packages/xchain-haven/package.json#L40

Pls add it to xchain-util as well (another PR is recommend, this PR ^ has already ~5k LOC).

* haven-core-js the underlying wallet library/sdk is included as a GitHub package as well for now. I'll create a npm package of it shortly.

Yes, pls add a npm package.

Thanks again!

packages/xchain-haven/README.md

packages/xchain-haven/__tests__/haven-core.test.ts

packages/xchain-haven/__tests__/openhaven-api.test.ts

packages/xchain-haven/package.json

packages/xchain-haven/src/client.ts

mikewyszinski · 2022-05-27T03:46:27Z

also can you merge the latest from master and add a changelog?

MartyHav · 2022-05-28T18:22:16Z

@veado Implemented all your remarks and added a new PR: #593 for bip 39 phrase converter. Just saw you wanted to have it in xchain-util and not in xchain-crypto, would be a quick change, we can sort it out there.

MartyHav · 2022-05-29T13:12:38Z

also can you merge the latest from master and add a changelog?

yes merged and added changelog. Should changelog be maintained while PR is ongoing?

veado

Nice PR! Some comments have been added...

yarn lint
...
✖ 29 problems (1 error, 28 warnings)

Pls don't forget to run yarn lint

veado · 2022-06-02T13:16:19Z

packages/xchain-haven/__tests__/client.test.ts

+  })
+
+  it('should not throw an error for setting a good phrase', () => {
+    expect(havenClient.setPhrase(bip39Mnemonic)).toBeUndefined


typo .toBeUndefined() (note missing ()) or better .not.toThrow()

veado · 2022-06-02T13:22:59Z

packages/xchain-haven/__tests__/client.test.ts

+  it('should send funds', async () => {
+    havenClient.setNetwork(Network.Testnet)
+    havenClient.setPhrase(bip39Mnemonic)
+    const amount = baseAmount(2223, 12)


It seems 12 is default decimal. Pls add const XHV_DECIMAL = 12 into utils.ts

veado · 2022-06-02T13:38:25Z

packages/xchain-haven/src/client.ts

+  }
+
+  async getTransactions(params?: TxHistoryParams): Promise<TxsPage> {
+    const asset: Asset | undefined = params?.asset ? assetFromString(params.asset)! : undefined


Avoid !, lint will tell you Forbidden non-null assertion.

const asset: Asset | null = params?.asset ? assetFromString(params.asset) : null

Or even better: TxHistoryParams in xchain-client needs to be updated to include asset?: Asset (not asset: string). Can be done by another PR.

But this PR would be a breaking change, right?

veado · 2022-06-02T13:40:14Z

packages/xchain-haven/__tests__/client.test.ts

+    // Limit should work
+    const xhvAsset = AssetXHV
+    const txPages = await havenClient.getTransactions({ address: 'ignored', asset: assetToString(xhvAsset) })
+    return expect(txPages.txs[0].asset).toEqual(AssetXHV)


Hint: To compare assets, use eqAsset from xchain-util

expect(eqAsset(txPages.txs[0].asset, AssetXHV)).toBeTruthy()

veado · 2022-06-02T13:44:36Z

packages/xchain-haven/__tests__/haven-core.test.ts

+  it('should transfer funds', async () => {
+    client.init(mnenomonic, NetTypes.testnet)
+    // is equal to 0.1 XHV
+    const transferAmount = new BigNumber(10).exponentiatedBy(12).dividedBy(100)


There are helpers in xchain-utils for doing similar things ^

const transferAmount: BaseAmount = assetToBase(assetAmount(0.1, 12))

assetAmount(0.1, 12) => 0.1 XHV

assetToBase converts it ^ to smallest unit => 10000000000

veado · 2022-06-02T14:26:48Z

packages/xchain-haven/src/haven/haven-core-client.ts

+    this.syncHandler.purge()
+    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    //@ts-ignore
+    clearInterval(this.pingServerIntervalID)


Try to follow eslint, e.g.

this.pingServerIntervalID && clearInterval(this.pingServerIntervalID)

veado · 2022-06-02T14:29:12Z

packages/xchain-haven/src/haven/haven-core-client.ts

+
+      const lockedBalance = balance.subtract(unlockedBalance)
+
+      havenBalance[assetType as HavenTicker] = {


Try to avoid assetType as HavenTicker (see previous comments to similar topic)

veado · 2022-06-02T14:31:53Z

packages/xchain-haven/src/haven/haven-core-client.ts

+
+    if (this.base_fee === undefined || this.fork_version === undefined) {
+      const version = await get_version()
+      this.fork_version = version.fork_version as number


No need to cast the result if get_version will return a proper type (not any). Pls check previous comment to missing API types.

veado · 2022-06-02T14:33:01Z

packages/xchain-haven/src/haven/haven-core-client.ts

+const updateStatus = (_status: any) => {
+  //console.log(status)
+}


Is updateStatus still needed ^?

veado · 2022-06-02T14:35:58Z

packages/xchain-haven/src/haven/sync-handler.ts

+    if (!(await this.isSyncing())) {
+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      //@ts-ignore
+      clearInterval(this.updateSyncProgressIntervalID) // eslint-disable-next-line @typescript-eslint/ban-ts-comment


No need to ignore/disable eslint: this.updateSyncProgressIntervalID might be undefined, just add a check before (similar you have done in purge)

Thorian1te

Looks good to me. 👍

MartyHav · 2022-06-12T11:29:55Z

Nice PR! Some comments have been added...
yarn lint
...
✖ 29 problems (1 error, 28 warnings)
Pls don't forget to run yarn lint

Fixed all lint warnings/errors. Also fixed/improved the code parts you addressed in your comments.
Thanks for the review so far!

…/xchainjs-lib into feature/haven

MartyHav marked this pull request as ready for review May 24, 2022 11:18

veado requested review from veado, mikewyszinski and Thorian1te May 26, 2022 16:23

mikewyszinski requested changes May 27, 2022

View reviewed changes

MartyHav requested a review from mikewyszinski May 28, 2022 18:26

veado requested changes Jun 2, 2022

View reviewed changes

Thorian1te reviewed Jun 3, 2022

View reviewed changes

MartyHav requested a review from veado June 12, 2022 12:56

MartyHav requested a review from Thorian1te June 28, 2022 09:15

MartyHav added 15 commits July 15, 2022 10:20

Update haven-core.test.ts

3322e68

Update client.test.ts

46e20ea

change comment

f4a0dde

adjustments

f345f68

Delete package-lock.json

59f90d0

Update haven-core.test.ts

71f4e79

add git packages at latest commit

cd756af

adjustments

dbcf372

integration tests

403990c

add mocks for api responses

d581eef

rename folder

c1832b2

Create axios-adapter.ts

ef413e5

add get_version mock

824d469

update mocks

271b938

Update haven-core-client.ts

f1ccac8

MartyHav added 28 commits October 7, 2022 10:04

remove comment

0ce7c79

Update jest.config.js

6a4114c

Create CHANGELOG.md

eff43a6

fix lint errors

e51ace6

add HAVEN_DECIMAL and convertToHavenMnemonic to utils

e21ac8e

improve test suite

43a4eb2

improve getExplorerUrl

c3dd617

change name

bdfd287

Update openhaven-api.test.ts

3bf5df3

add unsubscriber

0db5a98

create asset dynamically

37d00e2

add guard for ticker

2ba35bc

Update client.ts

d69880d

remove unsafe type conversions

1925e4f

use type guard

58c2ff7

bug fix

c68c45f

add test for invalid asset in transfer

9eb6f46

Update haven-core.test.ts

3959240

Update package.json

3ce5db9

redesign client to address async actions at initalization

36e5e05

add guard for type safety

108f9ed

format file

27771e3

add test for unsubscribe

c10fe8c

Update CHANGELOG.md

47fc66c

Update index.ts

fd6aa42

upgrade deps

bba0973

bug fix

8e8a0ef

Merge branch 'feature/haven' of https://github.com/haven-protocol-org…

b9f678a

…/xchainjs-lib into feature/haven

github-actions bot added the stale label Sep 20, 2023

github-actions bot closed this Sep 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add xchain-haven #588

add xchain-haven #588

MartyHav commented May 24, 2022 •

edited

Loading

veado commented May 26, 2022 •

edited

Loading

mikewyszinski commented May 27, 2022

MartyHav commented May 28, 2022

MartyHav commented May 29, 2022

veado left a comment

veado Jun 2, 2022

veado Jun 2, 2022

veado Jun 2, 2022

MartyHav Jun 12, 2022

veado Jun 2, 2022

veado Jun 2, 2022

veado Jun 2, 2022

veado Jun 2, 2022

veado Jun 2, 2022

veado Jun 2, 2022

veado Jun 2, 2022

Thorian1te left a comment

MartyHav commented Jun 12, 2022


		const lockedBalance = balance.subtract(unlockedBalance)

		havenBalance[assetType as HavenTicker] = {

add xchain-haven #588

add xchain-haven #588

Conversation

MartyHav commented May 24, 2022 • edited Loading

veado commented May 26, 2022 • edited Loading

mikewyszinski commented May 27, 2022

MartyHav commented May 28, 2022

MartyHav commented May 29, 2022

veado 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

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

Thorian1te left a comment

Choose a reason for hiding this comment

MartyHav commented Jun 12, 2022

MartyHav commented May 24, 2022 •

edited

Loading

veado commented May 26, 2022 •

edited

Loading