-
Notifications
You must be signed in to change notification settings - Fork 103
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
chore(ecocredit/core): keeper method audit #1160
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1160 +/- ##
==========================================
+ Coverage 64.02% 69.60% +5.57%
==========================================
Files 218 221 +3
Lines 21174 23056 +1882
==========================================
+ Hits 13557 16047 +2490
+ Misses 6296 5626 -670
- Partials 1321 1383 +62
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
Looks good to me.
I was not sure at first on whether or not we should define Credits
separately or define within each message but if for some reason we need to change Credits
specifically for one message, we could theoretically redefine Credits
as needed specifically for that message and avoid breaking changes to the other messages that make use of Credits
.
I also don't think this would be a likely scenario and if we were to make changes to Credits
, we would likely need to make the same changes to the other uses. We might want to do something similar for BatchIssuance
, which I can do in #1161.
Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
CI is breaking. Looks like unexpected error messages. |
looks like there was an odd edge case in |
@aleem1314 can you review when you get a chance? 🙏 |
Opened #1181 |
cc @technicallyty can you resolve conflicts? 🙏 |
@@ -51,7 +51,7 @@ func (k Keeper) CreateBatch(ctx context.Context, req *core.MsgCreateBatch) (*cor | |||
return nil, err | |||
} | |||
|
|||
startDate, endDate := timestamppb.New(req.StartDate.UTC()), timestamppb.New(req.EndDate.UTC()) | |||
startDate, endDate := timestamppb.New(*req.StartDate), timestamppb.New(*req.EndDate) |
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.
I missed this the first time around... I don't think we want to store local times if the user provides a local time. I think we always want to store the UTC time.
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.
cc @clevinson
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.
whoops, nice catch - fixed here 524e2f5
Description
Bridge
keeper method.not found
error.timestamppb
conversions inCreateBatch
assertClassIssuer
to use naming ofkey
instead ofid
.GetCreditTypeFromBatchDenom
could just beCreditTypeTable.Get
ref: #715
Closes: #1171
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.md
Reviewers 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