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): listVaultHistory RPC #834

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

jingyi2811
Copy link
Contributor

What kind of PR is this?:
/kind feature

What this PR does / why we need it:
Add getVaultHistory rpc

Which issue(s) does this PR fixes?:
Fixes part of #488

Additional comments?:

@codeclimate
Copy link

codeclimate bot commented Nov 17, 2021

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

View more on Code Climate.

@codecov
Copy link

codecov bot commented Nov 18, 2021

Codecov Report

Merging #834 (efa469e) into main (2ba2c7f) will decrease coverage by 2.59%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #834      +/-   ##
==========================================
- Coverage   95.74%   93.14%   -2.60%     
==========================================
  Files         140      140              
  Lines        4485     4493       +8     
  Branches      586      587       +1     
==========================================
- Hits         4294     4185     -109     
- Misses        191      306     +115     
- Partials        0        2       +2     
Impacted Files Coverage Δ
packages/jellyfish-api-core/src/category/loan.ts 94.20% <100.00%> (-5.80%) ⬇️
...s/src/containers/RegTestContainer/LoanContainer.ts 100.00% <100.00%> (ø)
packages/jellyfish-wallet/src/wallet.ts 20.00% <0.00%> (-80.00%) ⬇️
packages/jellyfish-testing/src/rawtx.ts 15.00% <0.00%> (-77.50%) ⬇️
packages/jellyfish-address/src/p2wsh.ts 48.27% <0.00%> (-51.73%) ⬇️
...nsaction-builder/src/txn/txn_builder_masternode.ts 60.00% <0.00%> (-40.00%) ⬇️
packages/jellyfish-address/src/script/P2WSH.ts 72.72% <0.00%> (-27.28%) ⬇️
...nsaction-builder/src/txn/txn_builder_governance.ts 66.66% <0.00%> (-23.81%) ⬇️
...action-builder/src/txn/txn_builder_icxorderbook.ts 71.42% <0.00%> (-21.43%) ⬇️
packages/jellyfish-testing/src/poolpair.ts 80.95% <0.00%> (-19.05%) ⬇️
... and 17 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 2ba2c7f...efa469e. Read the comment docs.

@jingyi2811 jingyi2811 changed the title Add getVaultHistory RPC Add listVaultHistory RPC Nov 18, 2021
@netlify
Copy link

netlify bot commented Nov 24, 2021

✔️ Deploy Preview for jellyfish-defi ready!

🔨 Explore the source changes: efa469e

🔍 Inspect the deploy log: https://app.netlify.com/sites/jellyfish-defi/deploys/619e8c92d7c7010008f724ed

😎 Browse the preview: https://deploy-preview-834--jellyfish-defi.netlify.app

@jingyi2811 jingyi2811 marked this pull request as ready for review November 24, 2021 18:54
import { MasterNodeRegTestContainer } from './Masternode'

/**
* @deprecated use MasterNodeRegTestContainer instead
*/
export class LoanMasterNodeRegTestContainer extends MasterNodeRegTestContainer {
constructor (masternodeKey: MasterNodeKey = RegTestFoundationKeys[0]) {
super(masternodeKey, 'defi/defichain:HEAD-b810eda')
Copy link
Contributor Author

@jingyi2811 jingyi2811 Nov 24, 2021

Choose a reason for hiding this comment

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

This PR is merged to master a few hours ago. However, when I connect to MasterNodeRegTestContainer directly, funny error returned.

* @param {number} [pagination.maxBlockHeight] Maximum block height
* @param {number} [pagination.depth] Maximum Depth, from the genesis block is the default
* @param {string} [pagination.token] Token
* @param {string} [pagination.txtype] Tx type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

txtype should be txType. We may need to inform C++ side to change this

}

interface CreateLoanScheme {
minColRatio: number
Copy link
Contributor Author

@jingyi2811 jingyi2811 Nov 24, 2021

Choose a reason for hiding this comment

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

C++ side return this as ratio. Need to tell them to use minColRatio like how other RPC does


interface CreateLoanScheme {
minColRatio: number
interestRate: BigNumber
Copy link
Contributor Author

@jingyi2811 jingyi2811 Nov 24, 2021

Choose a reason for hiding this comment

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

C++ side return this as rate. Need to tell them to use interestRate like how other RPC does

Copy link
Contributor Author

@jingyi2811 jingyi2811 Nov 24, 2021

Choose a reason for hiding this comment

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

interestRate currently is a whole number. Need to tell C++ side to change to the number with 8 decimal places.

import { GenesisKeys, LoanMasterNodeRegTestContainer } from '@defichain/testcontainers'
import { ListVaultHistory } from '../../../src/category/loan'

describe('Loan listVaultHistory', () => {
Copy link
Contributor Author

@jingyi2811 jingyi2811 Nov 24, 2021

Choose a reason for hiding this comment

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

We should add all the possible scenarios for loan to this PR

expect(vaultHistory[9].blockHeight).toStrictEqual(auctionBidBlockHeight1)
expect(vaultHistory[9].amounts).toStrictEqual(['-5000.00000000@AAPL'])

if (vaultHistory[10].address === bobColAddr) {
Copy link
Contributor Author

@jingyi2811 jingyi2811 Nov 24, 2021

Choose a reason for hiding this comment

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

2 entries are made if a bid outbid by another bid.
However the order is inconsistent.
Need to tell this to C++ side.


it('should listAuctionHistory with depth only', async () => {
{
const vaultHistory = await alice.rpc.loan.listVaultHistory(createVaultTxId, { depth: 1 })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If pass in depth = 2, the RPC returns 1 record too.
I am not sure why.

Copy link
Contributor

@surangap surangap left a comment

Choose a reason for hiding this comment

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

@kresovic-milos I see a lot of comments from @jingyi2811 about inconsistencies in this RPC. Could you test these, and based on your results compile a list? Then we can share with cpp side.

@fuxingloh fuxingloh changed the title Add listVaultHistory RPC feat(jellyfish-api-core): listVaultHistory RPC Dec 22, 2021
@fuxingloh fuxingloh marked this pull request as draft December 30, 2021 06:58
@jingyi2811 jingyi2811 closed this Apr 6, 2022
@jingyi2811 jingyi2811 deleted the jimmy/rpc-getVaultHistory branch April 6, 2022 02:37
canonbrother pushed a commit that referenced this pull request Jun 1, 2022
@jingyi2811 jingyi2811 restored the jimmy/rpc-getVaultHistory branch June 2, 2022 07:46
@jingyi2811 jingyi2811 reopened this Jun 2, 2022
@codeclimate
Copy link

codeclimate bot commented Jun 2, 2022

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

View more on Code Climate.

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.

3 participants