refactor: remove class&nft id validation #13836
Conversation
dreamer-zq
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #13836 +/- ##
==========================================
+ Coverage 56.46% 56.68% +0.22%
==========================================
Files 648 642 -6
Lines 56221 55136 -1085
==========================================
- Hits 31747 31256 -491
+ Misses 21909 21350 -559
+ Partials 2565 2530 -35
|
There was a problem hiding this comment.
Can we instead update the validation rule to only check if not empty?
This way this avoids duplicating the error messages, and we are sure we validate the same way everywhere.
+1 for the changelog too.
EDIT: Actually, we could as well just set these errors (errors.Wrap(ErrInvalidID, "empty class id") and errors.Wrap(ErrInvalidID, "empty nft id")) as variable somewhere and use them.
julienrbrt
added
the
backport/v0.47.x
There was a problem hiding this comment.
lgtm! Thank you!
Maybe in addition of the ADR changes, we could mention in the specs (https://github.com/cosmos/cosmos-sdk/blob/main/x/nft/README.md) that the app dev is supposed to handle the validation and in the UPGRADING.md.
I can do that in a follow-up too.
| { | ||
| name: "class id does not exist", | ||
| args: struct { | ||
| ClassID string | ||
| ID string | ||
| }{ | ||
| ClassID: "class", | ||
| ID: ExpNFT.Id, | ||
| }, | ||
| expectErr: true, | ||
| errorMsg: "not found nft", | ||
| }, |
There was a problem hiding this comment.
nit: I think this one can be put back
There was a problem hiding this comment.
Just tested locally and it can be put back, but we can do a follow-up PR :)
There was a problem hiding this comment.
Merging then, I'll update the docs in a quick follow-up and re-add this test.
(cherry picked from commit 5f4350d)
Description
Closes: #13819
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