-
Notifications
You must be signed in to change notification settings - Fork 6
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: Pull balances /w block-height | fetch tokenPrice for native token #119
Conversation
c072e38
to
ec1212b
Compare
chainName: "klaytn", | ||
coingeckoId: "klay-token", | ||
symbol: "KLAY", | ||
tokenContract: "", |
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.
Note: we are not using tokenContract
, only coingeckoId
|
||
return { | ||
...balance, | ||
address, | ||
formattedBalance, | ||
tokens: [], | ||
symbol: this.chainConfig.nativeCurrencySymbol, | ||
...(tokenUsdPrice && { |
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.
since we are introducing the blockHeight
parameter when asking for balances, does it make sense to include the block height at which that balance was observed as a return value?
For example:
return {
...balance,
address,
formattedBalance,
blockHeight,
tokens: [],
symbol: this.chainConfig.nativeCurrencySymbol,
...(tokenUsdPrice && {
balanceUsd: Number(formattedBalance) * tokenUsdPrice,
tokenUsdPrice
})
};
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.
we are actually injecting blockHeight
in base-wallet.ts
in pullNativeBalance
method, but we can definitely move it here.
walletBalanceConfig
) in wallet config to control pull balanceson-demand
vsscheduled
behaviourpriceFeedConfig
per chain andpriceFeedOptions
to control fetching token-priceon-demand
vsscheduled
behaviourpullBalances
andpullBalancesAtBlockHeight
getBlockHeight
as public method on walletManager instanceNote: For those who would think using
bigint
would have been better thanNumber
. Initially, I was usingbigint
forbalances
andtokenPrice
across the code, but we were losing simplicity in the code and it was more prone to conversion errors (eg: stringified number to bigint). Though, addingpricePrecision
was an option totokenPrice
but data can become unusable when it will be required to convert the balances in tokenAmount with decimals to actual USD amount without decimals, which can become bottleneck while calculating profits and loss and visualizing balances in grafana.Result of
pullBalancesAtBlockHeight
: