refactor(x/ecocredit): add batch contract mapping #1226
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1226 +/- ##
==========================================
- Coverage 79.34% 74.23% -5.11%
==========================================
Files 228 225 -3
Lines 21423 19519 -1904
==========================================
- Hits 16998 14490 -2508
- Misses 3064 3737 +673
+ Partials 1361 1292 -69
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -65,6 +65,7 @@ func (k Keeper) Cancel(ctx context.Context, req *core.MsgCancel) (*core.MsgCance | |||
| Address: owner, | |||
| TradableAmount: userBalTradable.String(), | |||
| RetiredAmount: userBalance.RetiredAmount, | |||
| EscrowedAmount: userBalance.EscrowedAmount, | |||
There was a problem hiding this comment.
Good thing we are updating these tests... critical issue resolved here.
There was a problem hiding this comment.
Good thing we are updating these tests... critical issue resolved here.
We have similar issue in send and retire. We are not updating sender and recipient EscrowedAmount send.
There was a problem hiding this comment.
Yea, it looks like we do. Let's address this in a separate pull request and make sure we update tests.
There was a problem hiding this comment.
I'll work on this today alongside migrating tests to gherkin/gocuke.
There was a problem hiding this comment.
Will be resolved in #1236
|
Aside from the changes that will follow #1225, this is ready for an initial review. 🙏 @clevinson @technicallyty |
There was a problem hiding this comment.
Nice set of test cases, this approach is looking much cleaner than what we've done previously.
Generally giving a concept ACK on this work.
One note - I couldn't find the logic for adding new contracts to the BatchContractTable. It sounds like this will be done once #1225 is merged in as that provides some of the necessary pieces to finish this up.
| // otherwise search for an existing project based on credit class id and | ||
| // project reference id and, if the project exists, create a credit batch | ||
| // under the existing project, otherwise, create a new project and then a | ||
| // new credit batch under the new project |
There was a problem hiding this comment.
👍 nice summary of the expected logic
| @@ -65,6 +65,7 @@ func (k Keeper) Cancel(ctx context.Context, req *core.MsgCancel) (*core.MsgCance | |||
| Address: owner, | |||
| TradableAmount: userBalTradable.String(), | |||
| RetiredAmount: userBalance.RetiredAmount, | |||
| EscrowedAmount: userBalance.EscrowedAmount, | |||
x/ecocredit/core/msg_bridge_test.go
Outdated
| }, | ||
| } | ||
|
|
||
| for msg, test := range tests { | ||
| t.Run(msg, func(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
Heads up. This was causing the same test to be run multiple times and not the others.
There was a problem hiding this comment.
Also see #1230 (comment)
There was a problem hiding this comment.
Reverted in 70e569a. This caused several errors unrelated to the scope of this pull request. Opening a separate pull request to address and will likely migrate to gherkin/gocuke.
| @@ -193,7 +193,6 @@ func TestMsgBridgeReceive_ValidateBasic(t *testing.T) { | |||
| } | |||
| for _, tt := range tests { | |||
| t.Run(tt.name, func(t *testing.T) { | |||
| t.Parallel() | |||
There was a problem hiding this comment.
Heads up. This was causing the same test to be run multiple times and not the others.
There was a problem hiding this comment.
Also see #1230 (comment)
There was a problem hiding this comment.
Reverted in 70e569a. This caused several errors unrelated to the scope of this pull request and the get and reset message functionality used within this test was not working as intended and was causing additional errors. Opening a separate pull request to address and will likely migrate to gherkin/gocuke.
|
cc @clevinson @technicallyty @aleem1314 ready for review 🙏 |
Description
Closes: #1127 / Closes: #1193 / Ref: #1206
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