-
Notifications
You must be signed in to change notification settings - Fork 20
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
(fix) Wrong symbol param for getPublicTrades request #626
Conversation
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.
The way this was map i dont think its entirely correct.
Not that this PR introduce the errors, but that the errors where displayed by last changes.
Logic should be the following:
1 - From Api we receive pairs, currencies, inactiveSymbols, mapSymbols, inactiveCurrencies
Pairs before being sent to backend should add the t as to get the tranding pair t${pair}
2 - As to display pairs, we should:
a - look if pair has its equivalent in mapSymbols
b - Split the pair into 2 currencies (get each currency from currencies array), this might have ':' as to split or be pair.length === 6 (3 and 3)
3 - Currencies have an ID and a name to display
4 - In movements display all the symbols
5 - For rest, filters the one that dont are in trading pairs.
6 - For the places now added ad f${symbol.id}
(only the filtered ones)
@@ -8,7 +8,7 @@ export * from './mapping' | |||
// BTCUSD -> tBTCUSD | |||
// BTCF0:USDF0 -> tBTCF0:USDF0 | |||
// USD -> fUSD | |||
const addPrefix = (symbol = '', isFunding = false) => ( | |||
export const addPrefix = (symbol = '', isFunding = false) => ( |
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.
So i would definitely change this.
What would do is the following:
1 - Add t to each result from pairs, that should be the trading pair.
const tradingPairs = pairs.map(p => `t${p}`)
@@ -61,7 +61,7 @@ export const getSymbolsURL = (symbols) => { | |||
|
|||
// BTC:USD -> BTCUSD | |||
// BTCF0:USTF0 -> BTCF0:USTF0 | |||
const deformatPair = pair => ((pair.length === 7) ? pair.replace(':', '') : pair) | |||
export const deformatPair = pair => ((pair.length === 7) ? pair.replace(':', '') : pair) |
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.
Same that comment below, there should not be a need to replace ':'
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.
Following our recent deliberations, we have decided to proceed with merging this pull request as an interim patch to address the immediate concern. Meanwhile, we will continue working towards a comprehensive solution that effectively resolves all legacy case scenarios.
Task: https://app.asana.com/0/1163495710802945/1204298582161856/f
Description:
symbol
param for thegetPublicTrades
request in some casesSymbol Filter
width to fit better for all available pairsBefore:
After: