-
Notifications
You must be signed in to change notification settings - Fork 2
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
add TestGetPrice #22
add TestGetPrice #22
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 idea for the unit test is good but we should make it clearer with table driven test struct like this example, I suggest to setup all the fail case first then the successful case after
} | ||
err := app.OracleKeeper.SetBandPriceState(ctx, "ATOM", bandPriceStateATOM) | ||
require.NoError(t, err) | ||
|
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 should add the check app.OracleKeeper.GetPrice
} | ||
err = app.OracleKeeper.SetBandPriceState(ctx, "USD", bandPriceStateUSD) | ||
require.NoError(t, err) | ||
|
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.
Here too
@@ -219,7 +219,7 @@ func (k *Keeper) GetAllBandPriceStates(ctx sdk.Context) []*types.BandPriceState | |||
func (k *Keeper) GetPrice(ctx sdk.Context, base, quote string) *math.LegacyDec { | |||
// query ref by using GetBandPriceState | |||
basePriceState := k.GetBandPriceState(ctx, base) | |||
if basePriceState == nil { | |||
if basePriceState == nil || basePriceState.Rate.IsZero() { |
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.
Nice catch for this 👍
price = app.OracleKeeper.GetPrice(ctx, "ATOM", "USD") | ||
require.Nil(t, price) | ||
|
||
// Case 5: Quote price is invalid (rate is zero), expect nil |
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.
This case seems wrong because when quote=USD
the code will return here
reserve/x/oracle/keeper/band_oracle.go
Line 226 in e4055d4
if quote == types.QuoteUSD { |
ATOM
at line 175 above with invalid rate
so maybe it return nil at here (we tested in case 4)reserve/x/oracle/keeper/band_oracle.go
Line 222 in 1230d5e
if basePriceState == nil || basePriceState.Rate.IsZero() { |
I recommend we should rewrite this test with table-driven test to easy keep track all the test case and follow the logic of each test case
No description provided.