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

feat(jellyfish-api-core): implement refundHtlcAll #1324

Merged
merged 4 commits into from
Apr 22, 2022

Conversation

cwkang1998
Copy link
Contributor

@cwkang1998 cwkang1998 commented Apr 5, 2022

What this PR does / why we need it:

Implements spv spv_refundhtlcall rpc call.

Which issue(s) does this PR fixes?:

Fixes part of #202

Additional comments?:

Need some confirmation with how the ain implementation of this rpc is. Right now ain's implementation returns an array of strings, containing only one string, shown here.

The rpc documentation from ain however shows that it returns a string txid, which is not the case.

This current implementation for jellyfish deals with this by simply taking the first element of the returned array.

Edit: This rpc now returns a string array, faithful to its ain implementation.

@codeclimate
Copy link

codeclimate bot commented Apr 5, 2022

Code Climate has analyzed commit d7c9a05 and detected 0 issues on this pull request.

View more on Code Climate.

@cwkang1998 cwkang1998 mentioned this pull request Apr 5, 2022
30 tasks
@netlify
Copy link

netlify bot commented Apr 5, 2022

Deploy Preview for jellyfishsdk ready!

Name Link
🔨 Latest commit d7c9a05
🔍 Latest deploy log https://app.netlify.com/sites/jellyfishsdk/deploys/625feb4f078e9e0008abeee6
😎 Deploy Preview https://deploy-preview-1324--jellyfishsdk.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

Merging #1324 (d7c9a05) into main (868b4b0) will increase coverage by 1.70%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1324      +/-   ##
==========================================
+ Coverage   88.21%   89.91%   +1.70%     
==========================================
  Files         348      350       +2     
  Lines       10030    10115      +85     
  Branches     1224     1244      +20     
==========================================
+ Hits         8848     9095     +247     
+ Misses       1113      966     -147     
+ Partials       69       54      -15     
Impacted Files Coverage Δ
packages/jellyfish-api-core/src/category/spv.ts 86.66% <100.00%> (+0.62%) ⬆️
packages/jellyfish-wallet-encrypted/src/hd_node.ts 12.12% <0.00%> (-87.88%) ⬇️
...kages/jellyfish-wallet-encrypted/src/encryption.ts 7.69% <0.00%> (-86.16%) ⬇️
apps/whale/src/module.database/_module.ts 78.57% <0.00%> (-21.43%) ⬇️
...le/src/module.indexer/model/dftx/composite.swap.ts 67.44% <0.00%> (-18.61%) ⬇️
...rc/module.api/interceptors/response.interceptor.ts 66.66% <0.00%> (-16.67%) ⬇️
...c/module.api/interceptors/exception.interceptor.ts 39.28% <0.00%> (-3.58%) ⬇️
packages/jellyfish-transaction/src/tx_composer.ts 78.33% <0.00%> (-1.67%) ⬇️
...s/legacy-api/src/controllers/PoolPairController.ts 96.26% <0.00%> (-0.84%) ⬇️
packages/jellyfish-api-core/src/index.ts 100.00% <0.00%> (ø)
... and 38 more

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 868b4b0...d7c9a05. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Apr 5, 2022

Docker build preview for jellyfish/apps is ready!

Built with commit aaaa01d

  • ghcr.io/defich/legacy-api:pr-1324
  • ghcr.io/defich/ocean-api:pr-1324
  • ghcr.io/defich/playground-api:pr-1324
  • ghcr.io/defich/status-api:pr-1324

Copy link
Contributor

@canonbrother canonbrother left a comment

Choose a reason for hiding this comment

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

LGTM!!
(Regarding the return arr from ain.. if that does really matter.. we'd open issue on ain)

@ivan-zynesis
Copy link
Contributor

LGTM!! (Regarding the return arr from ain.. if that does really matter.. we'd open issue on ain)

Hmm prefer to return array instead for consistencies. I am thinking if ain side is a bug, what if the wallet has multiple htlc, then it should return txid of each refunded.

canonbrother
canonbrother previously approved these changes Apr 8, 2022
@cwkang1998
Copy link
Contributor Author

cwkang1998 commented Apr 8, 2022

LGTM!! (Regarding the return arr from ain.. if that does really matter.. we'd open issue on ain)

Hmm prefer to return array instead for consistencies. I am thinking if ain side is a bug, what if the wallet has multiple htlc, then it should return txid of each refunded.

Does make sense that it'll want to return all the transactions made for the refund. But not sure when ain will implement this.

If so then I think I'll make it return a string array, but I'll just note it somewhere that there is this inconsistency then.

ivan-zynesis
ivan-zynesis previously approved these changes Apr 8, 2022
canonbrother
canonbrother previously approved these changes Apr 8, 2022
@cwkang1998 cwkang1998 enabled auto-merge (squash) April 13, 2022 10:38
@cwkang1998 cwkang1998 dismissed stale reviews from canonbrother and ivan-zynesis via d7c9a05 April 20, 2022 11:15
@cwkang1998 cwkang1998 merged commit 491b017 into main Apr 22, 2022
@cwkang1998 cwkang1998 deleted the chen/refund_htlcall_rpc branch April 22, 2022 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants