Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

blockchain.getRawMempool, wallet.getNewAddress, wallet.getAddressInfo, wallet.validateAddress, wallet.listAddressGroupings, wallet.getWalletInfo, wallet.setWalletFlag, wallet.sendToAddress, wallet.createWallet #112

Merged
merged 15 commits into from
Apr 20, 2021

Conversation

canonbrother
Copy link
Contributor

@canonbrother canonbrother commented Apr 13, 2021

What kind of PR is this?:

/kind feature

What this PR does / why we need it:

Implemented type for RPC: #48

  1. blockchain.getRawMempool
  2. wallet.getNewAddress
  3. wallet.getAddressInfo
  4. wallet.validateAddress
  5. wallet.listAddressGroupings
  6. wallet.getWalletInfo
  7. wallet.setWalletFlag
  8. wallet.sendToAddress
  9. wallet.createWallet

@netlify
Copy link

netlify bot commented Apr 13, 2021

Deploy preview for jellyfish-defi ready!

Built with commit 7a45c00

https://deploy-preview-112--jellyfish-defi.netlify.app

@defichain-bot defichain-bot added the kind/feature New feature request label Apr 13, 2021
@github-actions
Copy link

github-actions bot commented Apr 13, 2021

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/jellyfish/dist/jellyfish.umd.js 17.96 KB (+4.86% 🔺) 360 ms (+4.86% 🔺) 170 ms (-1.84% 🔽) 529 ms

@canonbrother canonbrother changed the title blockchain.getRawMempool, wallet.createWallet, wallet.getNewAddress, wallet.getAddressInfo, wallet.validateAddress, wallet.sendToAddress blockchain.getRawMempool, wallet.getNewAddress, wallet.getAddressInfo, wallet.validateAddress, wallet.listAddressGroupings, wallet.getWalletInfo, wallet.setWalletFlag, wallet.sendToAddress, wallet.createWallet Apr 13, 2021
@canonbrother canonbrother marked this pull request as ready for review April 14, 2021 05:21
@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #112 (7a45c00) into main (515a981) will increase coverage by 0.24%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
+ Coverage   94.46%   94.70%   +0.24%     
==========================================
  Files          40       40              
  Lines         831      888      +57     
  Branches       95      108      +13     
==========================================
+ Hits          785      841      +56     
- Misses         44       45       +1     
  Partials        2        2              
Impacted Files Coverage Δ
packages/jellyfish-api-core/src/category/mining.ts 100.00% <ø> (ø)
packages/jellyfish-api-core/src/category/rawtx.ts 100.00% <ø> (ø)
...es/testcontainers/src/chains/reg_test_container.ts 98.00% <ø> (ø)
...ages/jellyfish-api-core/src/category/blockchain.ts 100.00% <100.00%> (ø)
packages/jellyfish-api-core/src/category/wallet.ts 100.00% <100.00%> (ø)
packages/jellyfish-transaction/src/tx_composer.ts 84.21% <0.00%> (-0.64%) ⬇️
...ellyfish-transaction/src/buffer/buffer_composer.ts 98.61% <0.00%> (+0.14%) ⬆️
packages/jellyfish-transaction/src/tx_segwit.ts 100.00% <0.00%> (+4.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 515a981...7a45c00. Read the comment docs.

Copy link
Contributor

@fuxingloh fuxingloh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, nice tests. Added a few minor details you should consider

packages/jellyfish-api-core/src/category/wallet.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/src/category/wallet.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/src/category/wallet.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/src/category/wallet.ts Outdated Show resolved Hide resolved
website/docs/jellyfish/api/wallet.md Outdated Show resolved Hide resolved
packages/jellyfish-api-core/src/category/blockchain.ts Outdated Show resolved Hide resolved
@fuxingloh fuxingloh self-requested a review April 19, 2021 13:11
Copy link
Contributor

@fuxingloh fuxingloh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments but I think this is good enough to be merged into main.

Comment on lines +28 to +30
* @param {CreateRawTxOptions=} options
* @param {number=} options.locktime Non-0 value also locktime-activates inputs
* @param {boolean=} options.replaceable Marks this transaction as BIP125-replaceable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use this syntax instead, we can standard this in the future

* @param {number} [options.locktime=0] Non-0 value...


```ts title="client.wallet.setWalletFlag()"
interface wallet {
setWalletFlag (flag: WalletFlag, value: boolean = true): Promise<WalletFlagResult>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WalletFlag enum missing in docs.


```ts title="client.wallet.getNewAddress()"
interface wallet {
getNewAddress (label: string = '', addressType = AddressType.BECH32): Promise<string>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AddressType enum missing in docs

Comment on lines +312 to +313
['witness_version']: number
['witness_program']: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

witness_version: number
witness_program: string

Comment on lines +38 to +42
return await waitForExpect(async () => {
const aliceAddress = await client.wallet.getNewAddress('alice')

expect(typeof aliceAddress).toBe('string')
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why you wrap everything in waitForExpect?

@fuxingloh fuxingloh merged commit 178cda8 into main Apr 20, 2021
@fuxingloh fuxingloh deleted the canonbrother/rpc-blockchain-getrawmempool branch April 20, 2021 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants