refactor(ecocredit/basket): wire core submodule into baskets #935
Conversation
|
might have to comment out sims temporarily, i believe aleem is working on integration of sims |
Codecov Report
@@ Coverage Diff @@
## master #935 +/- ##
==========================================
- Coverage 72.96% 72.89% -0.08%
==========================================
Files 197 196 -1
Lines 23380 23050 -330
==========================================
- Hits 17060 16802 -258
+ Misses 4990 4946 -44
+ Partials 1330 1302 -28
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Looking good.
The code coverage diff has dropped with param validation but given this is copy paste and we are planning to update to new params, I think that's ok for now.
Let's comment out the lines suggested rather than the whole simulation test and we can renable when updated.
A few small suggestions otherwise and a question about the commented out basket scenario, i.e. what's missing before we can re-enable?
| require.NoError(err) | ||
| require.Equal(creditsToDeposit.String(), cbRes.RetiredAmount) | ||
| } | ||
| // TODO: reimpl after full submodule integration |
There was a problem hiding this comment.
What items are missing before full submodule integration?
There was a problem hiding this comment.
params, ops/sims. this test was failing since i started using core.Params, and the integration test/app is wired up for v1alpha params
There was a problem hiding this comment.
This is not ideal but we can handle in a separate pull request when we refactor tests #728.
ryanchristo
changed the title
Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
x/ecocredit/server/basket/take.go
Outdated
| @@ -212,3 +206,88 @@ func (k Keeper) addCreditBalance(ctx context.Context, owner sdk.AccAddress, batc | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func (k Keeper) addAndSaveBalance(ctx context.Context, user sdk.AccAddress, batchId uint64, amount math.Dec) error { | |||
There was a problem hiding this comment.
should some of these be helper functions in core?
There was a problem hiding this comment.
yeah that would make more sense. added to core/utils, will merge now
Description
this PR also comments out the integration test for baskets as it is currently broken until we fully wire everything together (#898). Currently the app is setup to deal with v1alpha1 params, whereas the new submodules are wired in for v1.
Closes: #926
Closes: #925
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...
!in the type prefix if API or client breaking change