Skip to content
This repository was archived by the owner on Apr 8, 2024. It is now read-only.

Feature/PI-387 use fetch instead of request promise#555

Merged
adrian-gierakowski merged 9 commits intodevfrom
feature/PI-387-use-fetch-instead-of-request-promise
Aug 31, 2023
Merged

Feature/PI-387 use fetch instead of request promise#555
adrian-gierakowski merged 9 commits intodevfrom
feature/PI-387-use-fetch-instead-of-request-promise

Conversation

@Dynki
Copy link
Copy Markdown

@Dynki Dynki commented Aug 31, 2023

In order to allow the portal to migrate to Vite, we had to patch code in the @rhino.fi/client-js package. This is not ideal and so this PR is to change client-js to allow the portal patches to be removed.

The following files have been updated.

  1. @rhino.fi/client-js/src/api/deposit.js
  2. @rhino.fi/client-js/src/api/estimatedNextBatchTime.js
  3. @rhino.fi/client-js/src/api/eth/getGasStationPrice.js
  4. @rhino.fi/client-js/src/api/fastWithdrawal.js
  5. @rhino.fi/client-js/src/api/fastWithdrawalFee.js
  6. @rhino.fi/client-js/src/api/getConfig.js
  7. @rhino.fi/client-js/src/api/getGasPrice.js
  8. @rhino.fi/client-js/src/api/getTickers.js
  9. @rhino.fi/client-js/src/api/getTokenHolders.js
  10. @rhino.fi/client-js/src/api/getTokenLiquidityLeft.js
  11. @rhino.fi/client-js/src/api/getTokenSaleStartEnd.js
  12. @rhino.fi/client-js/src/api/ledger/deposit.js
  13. @rhino.fi/client-js/src/api/ledger/transferUsingVaultIdAndStarkKey.js
  14. @rhino.fi/client-js/src/api/ledger/withdraw.js
  15. @rhino.fi/client-js/src/api/submitMarketOrder.js
  16. @rhino.fi/client-js/src/api/submitOrder.js
  17. @rhino.fi/client-js/src/api/transferUsingVaultIdAndStarkKey.js
  18. @rhino.fi/client-js/src/api/withdraw.js
  19. @rhino.fi/client-js/src/lib/dvf/post-generic.js
  • The post-generic file has been updated to use request from dvf_utils
  • The remaining 18 files have been updated to use either the post-generic or get-generic functions

Comment thread src/api/estimatedNextBatchTime.js Outdated
const url = `${dvf.config.api}/v1/trading/r/estimatedNextBatchTime?t=${t}`
try {
const data = await get(url)
const data = await get(dvf, url)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

generic-get prepends dvf.config.api to the 2nd arg so if you want to use it we should remove do:

const url = `/v1/trading/r/estimatedNextBatchTime?t=${t}`

const url = dvf.config.api + endpoint

Comment thread src/api/estimatedNextBatchTime.js Outdated
try {
const data = await get(url)
const data = await get(dvf, url)
return JSON.parse(data)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I did a quick test and get-generic will return parsed data so we should just return data here

Comment thread src/api/estimatedNextBatchTime.js Outdated
const data = await get(dvf, url)
return JSON.parse(data)
}
catch(e) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the catch here is unnecessary, so pulling all the comment together the function should become:

module.exports = async dvf => {
  // avoid browser cache with timestamp as querystring
  const t = Date.now()
  const url = `/v1/trading/r/estimatedNextBatchTime?t=${t}`
  return get(dvf, url)
}

Comment thread src/api/eth/getGasStationPrice.js Outdated
module.exports = async (dvf) => {
try {
const res = await get(`${dvf.config.gasApi}/json/ethgasAPI.json?api-key=${dvf.config.gasStationApiKey || ''}`)
const res = await get(dvf, `${dvf.config.gasApi}/json/ethgasAPI.json?api-key=${dvf.config.gasStationApiKey || ''}`)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

given the previous comments, we cannot use get-generic here as it prepend dvf.config.api and we need dvf.config.gasApi here, so let's switch to plain request.get from @rhino.fi/dvf-utils

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Do we need to parse the response in the following line then? As it looks like we don't, but I'm not sure

dvf.config.defaultGasPrice = parseInt((JSON.parse(res).average * 1.25 *100000000))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess not, If you remove the JSON.parse I can test to confirm

Comment thread src/api/fastWithdrawal.js Outdated
const post = require('../lib/dvf/post-generic')

module.exports = async (dvf, withdrawalData) => {
const url = dvf.config.api + '/v1/trading/w/fastWithdrawal'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as above

Suggested change
const url = dvf.config.api + '/v1/trading/w/fastWithdrawal'
const url = '/v1/trading/w/fastWithdrawal'

Comment thread src/api/ledger/withdraw.js Outdated
@@ -31,7 +31,7 @@ module.exports = async (dvf, token, amount, starkWithdrawal) => {
//console.log({ data })
const url = dvf.config.api + '/v1/trading/w/withdraw'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as above

Comment thread src/api/submitMarketOrder.js Outdated
const { value, error } = schema.validate(orderData)
// TODO handle error
return post(dvf.config.api + '/v1/trading/w/submitOrder', {
return post(dvf, dvf.config.api + '/v1/trading/w/submitOrder', {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as above

Comment thread src/api/submitOrder.js Outdated
)

return post(dvf.config.api + '/v1/trading/w/submitOrder', { json })
return post(dvf, dvf.config.api + '/v1/trading/w/submitOrder', { json })
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as above

const post = require('../lib/dvf/post-generic')

module.exports = async (dvf, transferData, feeRecipient) => {
const url = dvf.config.api + '/v1/trading/w/transfer'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as above

Comment thread src/api/withdraw.js Outdated
}
//console.log({ data })
return post(url, { json: data })
return post(dvf, url, { json: data })
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as above

Copy link
Copy Markdown
Contributor

@adrian-gierakowski adrian-gierakowski left a comment

Choose a reason for hiding this comment

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

did some more testing and it turns out that dvf-utils request has slightly different interface then request-promise so src/lib/dvf/post-generic.js shoud be changed to:

const { request } = require('@rhino.fi/dvf-utils')
const _omitBy = require('lodash/omitBy')
const _isNil = require('lodash/isNil')

module.exports = async (dvf, endpoint, json = {}, headers = {}) => {
  const url = dvf.config.api + endpoint

  const options = {
    headers,
    // removes null and undefined values
    data: _omitBy(json, _isNil)
  }

  return request.post(url, options)
}

@adrian-gierakowski adrian-gierakowski merged commit 7faf8d4 into dev Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants