refactor: improve leverage keeper tests and errors#1300
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1300 +/- ##
==========================================
+ Coverage 51.65% 51.67% +0.02%
==========================================
Files 68 68
Lines 6870 6697 -173
==========================================
- Hits 3549 3461 -88
+ Misses 3063 2969 -94
- Partials 258 267 +9
|
|
Note: markdown link test fail was http |
robert-zaremba
left a comment
robert-zaremba
left a comment
There was a problem hiding this comment.
late review. Good job a nice improvements. Left few small things / suggestions.
| s.Require().Equal(sdk.NewInt64Coin(uDenom, 0), collateral) | ||
| // get u/umee collateral amount of empty account address | ||
| collateral := s.tk.GetCollateralAmount(ctx, sdk.AccAddress{}, uDenom) | ||
| require.Equal(coin(uDenom, 0), collateral) |
There was a problem hiding this comment.
coin could be a keyword :D
| fmt.Sprintf("amount of uToken exchange rates lower than one %d\n%s", count, msg), | ||
| ), broken | ||
| } | ||
| } |
There was a problem hiding this comment.
do we really want to check it after each block by every validator? I think we should have that function only invalidate genesis.
There was a problem hiding this comment.
Some of these would be expensive computationally to do each block, but having checked other modules (e.g. x/bank invariants) I see that iteration over balances is done there as well.
Is there some setting that validators are using to run invariants less often as they operate other chains?
There was a problem hiding this comment.
No, there is no such setting. But we can code it. The x/bank invariants are often disabled, because they take lot of time.
There was a problem hiding this comment.
BTW, the bank invariants are not even registered in the simapp.
|
|
||
| // check invariants | ||
| _, broken := keeper.ReserveAmountInvariant(app.LeverageKeeper)(ctx) | ||
| require.False(broken) |
There was a problem hiding this comment.
we should also test negative scenarios
| // Note: Setting umee liquidation threshold to 0.05 to make the user eligible for liquidation | ||
| umeeToken := newToken("uumee", "UMEE") | ||
| umeeToken.CollateralWeight = sdk.MustNewDecFromStr("0.05") | ||
| umeeToken.LiquidationThreshold = sdk.MustNewDecFromStr("0.05") |
There was a problem hiding this comment.
this 3 lines are repeated often. Could be better to create a function: newUmeeToken(collateraW, liquidationT)
| } | ||
|
|
||
| // newAccount creates a new account for testing, and funds it with any input tokens. | ||
| func (s *IntegrationTestSuite) newAccount(funds ...sdk.Coin) sdk.AccAddress { |
4 participants
Description
Except small behavior changes, this PR is limited to
x/leverage/keeper/*_test.goBehavior changes:
Main improvements:
x/leverage/client/testswhich would make it hard to identify the root cause of a failures.initBorrowScenarioin favor of more specific test setup functions (e.g.s.borrow)keeper_test.gofunctionsrequire.ErrorIs(err, types.ErrNotRegisteredToken)Cosmetic improvements:
400_000000when creating400 UMEEexpectedvalue is always on the left inrequire.Equal(expected,actual)for accurate test fail messagescloses: #1310
closes: #1242
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...