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

Update Balanc3 API #5744

Merged
merged 2 commits into from
Nov 13, 2018
Merged

Update Balanc3 API #5744

merged 2 commits into from
Nov 13, 2018

Conversation

bitpshr
Copy link
Contributor

@bitpshr bitpshr commented Nov 13, 2018

This pull request updates the Balanc3 URL used by the TokenRatesController to fetch token-to-ETH exchange rates. The controller now uses a bulk pricing endpoint that allows all exchange rates to be fetched in a single HTTP request.

Resolves #5741
Resolves #5062

@metamaskbot
Copy link
Collaborator

Builds ready [d30127d]: mascara, chrome, firefox, edge, opera

const query = pairs.join('&')
if (this._tokens.length > 0) {
try {
const response = await fetch(`https://exchanges.demo.balanc3.net/pie?${query}&autoConversion=true`)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the demo part of this URL? Is this a temporary API?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at #5741 (comment) we should be able to drop it?

Copy link
Contributor

@brunobar79 brunobar79 left a comment

Choose a reason for hiding this comment

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

Looks good! Just one minor comment and I also think @whymarrh has a point about the url, unless we're both missing some context.

if (!token) return log.error(`TokenRatesController - invalid tokens state:\n${JSON.stringify(tokens, null, 2)}`)
const address = token.address
contractExchangeRates[address] = await this.fetchExchangeRate(address)
const pairs = this._tokens.map(token => `pairs[]=${token.address}/ETH`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a 100% sure about this but since we support any base currency now (under settings). Shouldn't we use the selected currency instead of hardcoding ETH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Looks like our token pricing has been broken since we landed that functionality (if the user changed their native currency from ETH.)

@metamaskbot
Copy link
Collaborator

Builds ready [1788d81]: mascara, chrome, firefox, edge, opera

@bitpshr
Copy link
Contributor Author

bitpshr commented Nov 13, 2018

Thanks for the quick review @brunobar79 and @whymarrh. It's a little worrisome to me that token rates were seemingly broken if a user changed their native currency. I verified a few places in the UI and it does look like the contractExchangeRates property was used in this manner: they were always multiplied by conversionRate, which is specific to nativeCurrency. This PR now fixes this issue as well.

@bitpshr bitpshr merged commit 0549782 into develop Nov 13, 2018
@bitpshr bitpshr deleted the balanc3-api branch November 13, 2018 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants