feat: Rarity endpoints return prices field with prices in various currencies #500
Conversation
Pull Request Test Coverage Report for Build 2295822557
💛 - Coveralls |
fzavalia
force-pushed
the
feat/rarity-endpoints-in-mana-converted
branch
from
deeede0
to
1b148eb
Compare
| return { | ||
| id: rarity.name, | ||
| name: rarity.name, | ||
| price: rarity.price.toString(), | ||
| maxSupply: rarity.maxSupply.toString(), | ||
| } |
There was a problem hiding this comment.
What if we return the currency of the price. If inUSD is not set, it's not clear which is the currency it's returning the price.
There was a problem hiding this comment.
Do you suggest adding another field such as currency: 'mana' | 'usd' to distinguish?
src/Rarity/Rarity.router.ts
Outdated
| return collectionAPI.fetchRarities() | ||
| } | ||
|
|
||
| const inUSD = req.query.inUSD === 'true' |
There was a problem hiding this comment.
What if we name this query parameter currency? We can later have different conversions if we want to.
| const inUSD = req.query.inUSD === 'true' | |
| const inUSD = req.query.currency === 'usd' |
src/Rarity/Rarity.router.spec.ts
Outdated
| }) | ||
| }) | ||
|
|
||
| // TODO: Tests for when rarities feature flag is enabled |
There was a problem hiding this comment.
Tests are missing here :)
|
@LautaroPetaccio Thanks for the feedback!
|
…taining the rarities
fzavalia
force-pushed
the
feat/rarity-endpoints-in-mana-converted
branch
from
08ab436
to
c9ebf41
Compare
|
@LautaroPetaccio |
src/Rarity/Rarity.router.ts
Outdated
| price: blockchainRarity.price, | ||
| originalPrice: graphRarity.price, | ||
| originalCurrency: Currency.USD, |
There was a problem hiding this comment.
As "Original" as name might be a bit confusing, what if we change this to a map?
As a map:
price: {
'MANA': '4000000000000000000',
'USD': '10000000000000000000'
}
|
@LautaroPetaccio Replaced "original" fields with "prices" field containing a Record of Currency -> Price |
Closes #490