feat: add methods for updating a credit class #539
Conversation
| return err | ||
| } | ||
|
|
||
| if len(m.Issuers) == 0 { |
There was a problem hiding this comment.
should we allow issuers to be empty in this method, so someone can wipe the issuer list?
There was a problem hiding this comment.
Good question. We require issuers when creating a credit class, so I would lean towards requiring issuers when updating a credit class. I could however see wiping issuers as a way of disabling the credit class but the admin could also assign themselves as the only issuer to accomplish.
There was a problem hiding this comment.
okay, leaving as is
| return nil, sdkerrors.ErrUnauthorized.Wrapf("you are not the administrator of this class") | ||
| } | ||
|
|
||
| cInfo.Metadata = req.Metadata |
There was a problem hiding this comment.
this should probably cost some gas as metadata can be variable length - doesnt look like we have a set hardcapped length for metadata length. we should also charge some gas based on the length itself
There was a problem hiding this comment.
We have a max length of 256 following #540. We should make sure the metadata does not exceed this max length by adding a check in validate basic.
Codecov Report
@@ Coverage Diff @@
## master #539 +/- ##
==========================================
+ Coverage 75.47% 76.01% +0.53%
==========================================
Files 104 104
Lines 13360 13764 +404
==========================================
+ Hits 10084 10463 +379
- Misses 2679 2696 +17
- Partials 597 605 +8
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Looking good. I think we should keep the update messages separate, which is the same as the group module. Also, we should check for metadata length following #540.
| return err | ||
| } | ||
|
|
||
| if len(m.Issuers) == 0 { |
There was a problem hiding this comment.
Good question. We require issuers when creating a credit class, so I would lean towards requiring issuers when updating a credit class. I could however see wiping issuers as a way of disabling the credit class but the admin could also assign themselves as the only issuer to accomplish.
| return nil, sdkerrors.ErrUnauthorized.Wrapf("you are not the administrator of this class") | ||
| } | ||
|
|
||
| cInfo.Metadata = req.Metadata |
There was a problem hiding this comment.
We have a max length of 256 following #540. We should make sure the metadata does not exceed this max length by adding a check in validate basic.
| testCases := []struct { | ||
| name string | ||
| msg ecocredit.MsgUpdateClassMetadata | ||
| expErr bool | ||
| }{ |
There was a problem hiding this comment.
We should add a test case for metadata exceeding max length.
There was a problem hiding this comment.
added a test for this in msgs_test.go
Description
Adds support for updating metadata, admin, and issuers for credit classes
Closes: #535
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