add optional parameter to sumSingleBalance #89
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0xngmi
reviewed
Nov 18, 2022
src/generalUtil.ts
Outdated
| (balances[token] as number) = prevBalance + balance; | ||
| const value = prevBalance + balance | ||
| isValidNumber(value) | ||
| balances[token] = Number(value).toString() |
There was a problem hiding this comment.
hmm, this might have side effect when I think about it, if someone had mixed sumSingleBalance and directly adding something to the balances object. like
sdk.util.sumSingleBalance(balances, 'tether', 500)
...
balances[tether] += 500
// would result in { tether: 500500 }
Maybe it is better to always convert it to a number? I dont strongly prefer number or string but thought it is better to stick to one type. Is there advantages using BigNumber type?
There was a problem hiding this comment.
i think somewhere in defillama-server we might have some code that is based on checking whether a value is a number or not within balances, and this change would change all the values that used to be numbers into strings
not sure about this tho, but would be good to check
0xngmi
reviewed
Nov 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Maybe
balances[token] ?? 0leads to unexpected behavior/bug as it would hide balance call failures where response isnull|undefined