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-transaction, jellyfish-transaction-builder): Add futureswap dftxs #1329

Merged
merged 33 commits into from
Apr 12, 2022

Conversation

surangap
Copy link
Contributor

@surangap surangap commented Apr 6, 2022

What this PR does / why we need it:

Add futureswap and withdrawfutureswap dftxs

Which issue(s) does this PR fixes?:

Fixes part of #1268

Additional comments?:

@codeclimate
Copy link

codeclimate bot commented Apr 6, 2022

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

View more on Code Climate.

@netlify
Copy link

netlify bot commented Apr 6, 2022

Deploy Preview for jellyfish-defi ready!

Name Link
🔨 Latest commit 04001e7
🔍 Latest deploy log https://app.netlify.com/sites/jellyfish-defi/deploys/625441265d50f3000895e864
😎 Deploy Preview https://deploy-preview-1329--jellyfish-defi.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 6, 2022

Codecov Report

Merging #1329 (04001e7) into main (d7819a5) will increase coverage by 4.61%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1329      +/-   ##
==========================================
+ Coverage   83.39%   88.01%   +4.61%     
==========================================
  Files         335      347      +12     
  Lines        9737    10027     +290     
  Branches     1195     1224      +29     
==========================================
+ Hits         8120     8825     +705     
+ Misses       1517     1127     -390     
+ Partials      100       75      -25     
Impacted Files Coverage Δ
...transaction-builder/src/txn/txn_builder_account.ts 100.00% <100.00%> (+40.00%) ⬆️
...ages/jellyfish-transaction/src/script/dftx/dftx.ts 98.52% <100.00%> (+1.51%) ⬆️
...lyfish-transaction/src/script/dftx/dftx_account.ts 100.00% <100.00%> (ø)
...ckages/jellyfish-transaction/src/script/mapping.ts 98.02% <100.00%> (+10.02%) ⬆️
...ners/src/containers/RegTestContainer/Persistent.ts 11.11% <0.00%> (-81.49%) ⬇️
...rc/module.api/interceptors/response.interceptor.ts 66.66% <0.00%> (-25.00%) ⬇️
apps/whale/src/module.api/loan.vault.service.ts 62.50% <0.00%> (-24.11%) ⬇️
...ages/jellyfish-api-core/src/category/governance.ts 90.90% <0.00%> (-9.10%) ⬇️
...c/module.api/interceptors/exception.interceptor.ts 39.28% <0.00%> (-7.15%) ⬇️
packages/jellyfish-api-jsonrpc/src/index.ts 97.67% <0.00%> (-2.33%) ⬇️
... and 73 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 d7819a5...04001e7. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Apr 6, 2022

Docker build preview for jellyfish/apps is ready!

Built with commit a8d1aa9

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

@surangap surangap force-pushed the surangap/dftx-futureswap branch from 2142f4b to e8f9741 Compare April 7, 2022 06:38
@jingyi2811 jingyi2811 marked this pull request as ready for review April 11, 2022 07:44
@surangap surangap marked this pull request as draft April 11, 2022 08:20
@surangap surangap marked this pull request as ready for review April 11, 2022 12:49
@jingyi2811 jingyi2811 changed the title feat(jellyfish-transaction, jellyfish-transaction-builder): Add futureswap dftx feat(jellyfish-transaction, jellyfish-transaction-builder): Add futureswap and withdrawfutureswap dftxs Apr 11, 2022
@jingyi2811
Copy link
Contributor

jingyi2811 commented Apr 11, 2022

This PR is good to be merged to main.

Before merging, it is good to add this test item
should create multiple dtoken to dusd futureswaps within the same block
(See futureswap.test.ts)

Maybe @surangap you have good reason to remove this in your dftx test file, kindly advise

@surangap
Copy link
Contributor Author

This PR is good to be merged to main.

Before merging, it is good to add this test item should create multiple dtoken to dusd futureswaps within the same block (See futureswap.test.ts)

Maybe @surangap you have good reason to remove this in your dftx test file, kindly advise

yeah two reasons

  1. sendTransaction() has a generate(1) inside it. But we can modify it.
  2. Also sendrawtransaction RPC only supports sending only one tx at a time. so no multiple tx send at the same time.
  3. Currently, fundAddress() also generates blocks inside. This is for the fees.

We need to update the above👆 functions to have it done within one block and check.

@surangap
Copy link
Contributor Author

surangap commented Apr 12, 2022

This PR is good to be merged to main.
Before merging, it is good to add this test item should create multiple dtoken to dusd futureswaps within the same block (See futureswap.test.ts)
Maybe @surangap you have good reason to remove this in your dftx test file, kindly advise

yeah two reasons

  1. sendTransaction() has a generate(1) inside it. But we can modify it.
  2. Also sendrawtransaction RPC only supports sending only one tx at a time. so no multiple tx send at the same time.
  3. Currently, fundAddress() also generates blocks inside. This is for the fees.

We need to update the above👆 functions to have it done within one block and check.

@jingyi2811
btw i have the half-baked test case for this. Currently facing some issues with funding for txs fees. Coz with our current mock provider, one utxo can be used only once for some reason. I remember try digging it in the past. I guess have to continue doing it.
anw let's merge this since downstream require a release.
will add this test later.
#1346

@fuxingloh fuxingloh changed the title feat(jellyfish-transaction, jellyfish-transaction-builder): Add futureswap and withdrawfutureswap dftxs feat(jellyfish-transaction, jellyfish-transaction-builder): Add futureswap dftxs Apr 12, 2022
@fuxingloh fuxingloh merged commit 6f37559 into main Apr 12, 2022
@fuxingloh fuxingloh deleted the surangap/dftx-futureswap branch April 12, 2022 15:44
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.

7 participants