-
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
Add URI and URIHash to the asset FT. #685
Conversation
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 22 of 22 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @ysv)
integration-tests/upgrade/ft_new_attributes_test.go
line 70 at r1 (raw file):
require.Equal(t, ftt.token, tokenRes.Token) // create a token with URI and URIHash
This part might be moved to modules
because we run them after upgrading anyway
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 @miladz68, @wojtek-coreum, and @ysv)
integration-tests/upgrade/ft_new_attributes_test.go
line 70 at r1 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
This part might be moved to
modules
because we run them after upgrading anyway
It is already there, in the modules. Do you propose to remove that creation?
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 @dzmitryhil, @miladz68, and @ysv)
integration-tests/upgrade/ft_new_attributes_test.go
line 70 at r1 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
It is already there, in the modules. Do you propose to remove that creation?
Yea, then doing it here doesn't make sense
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 @miladz68, @wojtek-coreum, and @ysv)
integration-tests/upgrade/ft_new_attributes_test.go
line 70 at r1 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
Yea, then doing it here doesn't make sense
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 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68 and @ysv)
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 21 of 22 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil and @miladz68)
integration-tests/upgrade/ft_new_attributes_test.go
line 68 at r2 (raw file):
}) requireT.NoError(err) require.Equal(t, ftt.token, tokenRes.Token)
should we assert that URI & URIHash are empty strings by default after upgrade ?
Or this is already done as side effect by comparing ftt.token & tokenRes.Token
?
x/asset/ft/genesis_test.go
line 50 at r2 (raw file):
SendCommissionRate: sdk.MustNewDecFromStr(fmt.Sprintf("0.%d", i+1)), Version: i, URI: fmt.Sprintf("https://my-class-meta.invalid/%d", i),
nit:
I can see that you use my-class-meta.invalid
but this URI is valid, isn't it ?
Because I usually use invalid in name, description etc to identify invalid objects/attributes etc
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 and @ysv)
integration-tests/upgrade/ft_new_attributes_test.go
line 68 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
should we assert that URI & URIHash are empty strings by default after upgrade ?
Or this is already done as side effect by comparingftt.token & tokenRes.Token
?
Already done as side effect by comparing.
x/asset/ft/genesis_test.go
line 50 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
nit:
I can see that you usemy-class-meta.invalid
but this URI is valid, isn't it ?
Because I usually use invalid in name, description etc to identify invalid objects/attributes etc
That's the standard we follow in all tests with the URI and URIHash, the .invalid
is just a domain name which doesn't exist.
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 @miladz68)
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 9 of 9 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68)
Description
Add URI and URIHash to the asset FT.
Reviewers checklist:
Authors checklist
This change is