-
Notifications
You must be signed in to change notification settings - Fork 35
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): DFI Payback of all dTokens #1190
Conversation
Code Climate has analyzed commit dd15230 and detected 5 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
✅ Deploy Preview for jellyfish-defi ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Codecov Report
@@ Coverage Diff @@
## main #1190 +/- ##
==========================================
+ Coverage 90.47% 90.72% +0.24%
==========================================
Files 332 332
Lines 9594 9615 +21
Branches 1174 1176 +2
==========================================
+ Hits 8680 8723 +43
+ Misses 863 849 -14
+ Partials 51 43 -8
Continue to review full report at Codecov.
|
Docker build preview for jellyfish/apps is ready! Built with commit b96e5a1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments were added. need to add tests to dftx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/DeFiCh/jellyfish/runs/5723133086?check_suite_focus=true#step:5:557
Wonder is FCR causing the test fail? Or simply test result simply inconsistent/flaky?
FCR affects the test cases that check the exceptions for 50% DFI/DUSD |
packages/jellyfish-transaction-builder/src/txn/txn_builder_loans.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good. minor comments added. once resolved should be good to go since downstream require the updated API.
Can address the burn info update here -> https://github.com/DeFiCh/jellyfish/pull/1190/files#r832985771 separately.
Co-authored-by: surangap <surangatco@gmail.com>
Co-authored-by: surangap <surangatco@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Foresee we should need rpc.gov.setGov(attr: GovAttributes)
soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
txn_builder_loans.ts
loan.paybackLoanV2(...)
not tested?
However, I think the critical path of the code has been sufficiently tested on the RPC side. Let's merge this in first so we can do a quick release for the downstream. We will wait for the PR for |
kind/feature
What this PR does / why we need it:
Enable payback of all dTokens with DFI and not just dUSD
Related to Ain#1111 in version 2.7.0
Which issue(s) does this PR fixes?:
Fixes #
Additional comments?: