-
Notifications
You must be signed in to change notification settings - Fork 27
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
Allow zero precision for ft tokens #787
Conversation
3140dcd
to
0612352
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #787 +/- ##
==========================================
+ Coverage 35.26% 35.28% +0.01%
==========================================
Files 171 171
Lines 47469 47481 +12
==========================================
+ Hits 16742 16754 +12
Misses 27482 27482
Partials 3245 3245
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)
integration-tests/modules/assetft_test.go
line 117 at r1 (raw file):
// TestAssetFTIssue tests issue functionality of fungible tokens. func TestAssetFTIssue_ZeroPrecision(t *testing.T) {
I suggest to merge this test with TestAssetFTIssue
.
I think we should move towards less code in our integration tests because they might become unmaintainable at some point.
you can change it to table test with 2 cases.
input args for tests is &assetfttypes.MsgIssue
and output to assert (the only thing I find different) is expectedMetadata := banktypes.Metadata
. Everything else seems to be same
x/asset/ft/keeper/keeper.go
line 320 at r1 (raw file):
// we are force to have single entry in denom units and also Display must be the // same as Base. if precision == 0 {
nit: maybe it makes sense to write unit-tests for this part if possible
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)
x/asset/ft/keeper/keeper.go
line 320 at r1 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
nit: maybe it makes sense to write unit-tests for this part if possible
agree
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.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @wojtek-coreum and @ysv)
integration-tests/modules/assetft_test.go
line 117 at r1 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
I suggest to merge this test with
TestAssetFTIssue
.
I think we should move towards less code in our integration tests because they might become unmaintainable at some point.you can change it to table test with 2 cases.
input args for tests is&assetfttypes.MsgIssue
and output to assert (the only thing I find different) isexpectedMetadata := banktypes.Metadata
. Everything else seems to be same
Done.
x/asset/ft/keeper/keeper.go
line 320 at r1 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
agree
Done.
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.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68 and @wojtek-coreum)
x/asset/ft/keeper/keeper_test.go
line 155 at r2 (raw file):
} func TestKeeper_Issue_ZeroPrecision(t *testing.T) {
sorry, my comment probably confused you.
I meant unit-tests for token issuance generally not only for Precision: 0,
case
Code quote:
TestKeeper_Issue_ZeroPrecision
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @wojtek-coreum and @ysv)
x/asset/ft/keeper/keeper_test.go
line 155 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
sorry, my comment probably confused you.
I meant unit-tests for token issuance generally not only forPrecision: 0,
case
I still don't get it, we already have tests for token issuance both in unit and integration tests.
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.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)
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.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)
Description
Allow zero precision for ft tokens
Reviewers checklist:
Authors checklist
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)