Migrate copied nft module to cosmos sdk nft module #666
Conversation
miladz68
requested review from
dzmitryhil,
ysv and
wojtek-coreum
and removed request for
a team
There was a problem hiding this comment.
Reviewed 52 of 52 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)
app/app.go line 198 at r1 (raw file):
feemodel.AppModuleBasic{}, wnft.AppModuleBasic{}, cnftmodule.AppModuleBasic{},
Why is it needed now? Isn't it the wnft module which does whatever is needed?
integration-tests/modules/assetnft_test.go line 1476 at r1 (raw file):
// TestAssetNFTAminoMultisig tests that assetnft module works seamlessly with amino multisig. func TestAssetNFTAminoMultisig(t *testing.T) {
why is it removed?
integration-tests/upgrade/nft_migration_test.go line 96 at r1 (raw file):
} func (nut *nftMigrationTest) After(t *testing.T) {
Try to send using new tx type
x/asset/nft/keeper/keeper.go line 138 at r1 (raw file):
id := types.BuildClassID(settings.Symbol, settings.Issuer) if err := nft.ValidateClassID(id); err != nil {
why removed?
x/asset/nft/types/token.go line 115 at r1 (raw file):
} if err := nft.ValidateNFTID(id); err != nil {
Why removed?
x/nft/keeper/grpc_query.go line 13 at r1 (raw file):
var _ nft.QueryServer = Keeper{} // Balance return the number of NFTs of a given class owned by the owner, same as balanceOf in ERC721.
returns
x/nft/keeper/grpc_query_test.go line 34 at r1 (raw file):
req = &nft.QueryBalanceRequest{} }, cosmosnft.ErrEmptyClassID.Error(),
are the error messages, and their codes and namespaces same as before? If not, then this is not backward compatible
x/nft/keeper/msg_server.go line 15 at r1 (raw file):
// Send implement Send method of the types.MsgServer. func (k Keeper) Send(goCtx context.Context, msg *nft.MsgSend) (*nft.MsgSendResponse, error) { _, err := k.wkeeper.Send(goCtx, &cosmosnft.MsgSend{
For other keeper methods you have wrappers in k where you convert nft struct between types. But here you do it on the caller side. I would prefer to stick to that decision and have wrapping Send method in k.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)
app/app.go line 198 at r1 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
Why is it needed now? Isn't it the
wnftmodule which does whatever is needed?
we need to be able to process coreum.nft.v1beta1 messages as legacy route. in the pas wnft handled that route, but now wnft is handling cosmos.nft.v1beta1 so we are intoruding this to handle the old legacy routes.
integration-tests/modules/assetnft_test.go line 1476 at r1 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
why is it removed?
we introduced the amino to the NFT module, but the orignial nft module does not implement it. so we are not able to support the Amino for NFT anymore.
integration-tests/upgrade/nft_migration_test.go line 96 at r1 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
Try to send using new tx type
we do that in a separate integration test.
x/asset/nft/keeper/keeper.go line 138 at r1 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
why removed?
the nft module has remove the validation criteria for class id. we have this check here, so our generated id will not class with the validation of the nft module. since nft module is not doing validation, this check is not needed.
x/asset/nft/types/token.go line 115 at r1 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
Why removed?
nft id validation was part of the nft module and it is now removed, so the only validation that must be enforced is the one introduced by assetnft module.
x/nft/keeper/grpc_query.go line 13 at r1 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
returns
this comments are copied from the cosmos sdk v0.45, and the grammar error exists in the original code as well. Originally we thought we should not try to fix them.
x/nft/keeper/grpc_query_test.go line 34 at r1 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
are the error messages, and their codes and namespaces same as before? If not, then this is not backward compatible
error codes are the same, but the error messages are not. I agree that this is breaking change, but this breaking change was introduced in v0.47 and probably exists all over the cosmos SDK codebase. so I don't think we can do much about it.
x/nft/keeper/msg_server.go line 15 at r1 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
For other keeper methods you have wrappers in
kwhere you convert nft struct between types. But here you do it on the caller side. I would prefer to stick to that decision and have wrappingSendmethod ink.
It is the same strategy between query and tx handlers where we convert the types and pass it down. It is not different, or maybe I don't fully get your comment ?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @ysv)
integration-tests/upgrade/nft_migration_test.go line 96 at r1 (raw file):
Previously, miladz68 (milad) wrote…
we do that in a separate integration test.
But we don't do this for nft minted before the upgrade
x/nft/keeper/msg_server.go line 15 at r1 (raw file):
Previously, miladz68 (milad) wrote…
It is the same strategy between query and tx handlers where we convert the types and pass it down. It is not different, or maybe I don't fully get your comment ?
Ignore this, my mind is still on holidays
There was a problem hiding this comment.
Reviewable status: 51 of 52 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)
integration-tests/upgrade/nft_migration_test.go line 96 at r1 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
But we don't do this for nft minted before the upgrade
Done.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 51 of 52 files reviewed, 7 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)
a discussion (no related file):
Please search for The name is updated from "nft" to "cnft" to keep an ability to migrate to sdk native module. comment. Is it still valid?
a discussion (no related file):
From what I understand currently we use old root store key with the new cosmos SDK module. IMO it would be nice to migrate to cosmos key as well, in order to drop the support completely from the next release. But that is not required.
integration-tests/modules/legacy_nft_assetnft_test.go line 123 at r2 (raw file):
// change the owner sendMsg := &nft.MsgSend{
You use nft.MsgSend but how about use the cosmosnft.MsgSend as well ? And same for the queries.
integration-tests/upgrade/nft_migration_test.go line 96 at r2 (raw file):
} func (nut *nftMigrationTest) After(t *testing.T) {
IMO in the after we should test both cosmos and legacy API to be sure that the data is still accesable with the legacy API after the migration.
x/asset/nft/keeper/keeper.go line 137 at r2 (raw file):
} id := types.BuildClassID(settings.Symbol, settings.Issuer)
Why don't we validate it anymore?
x/asset/nft/types/token.go line 115 at r2 (raw file):
} if err := nft.ValidateNFTID(id); err != nil {
Why don't we validate it anymore?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @miladz68 and @ysv)
There was a problem hiding this comment.
Reviewable status: 51 of 53 files reviewed, 6 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)
a discussion (no related file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
Please search for
The name is updated from "nft" to "cnft" to keep an ability to migrate to sdk native module.comment. Is it still valid?
that comment is part of the legacy module. it is ok to keep it to understand why we did this, and later we will remove the whole module.
a discussion (no related file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
From what I understand currently we use old root store key with the new cosmos SDK module. IMO it would be nice to migrate to cosmos key as well, in order to drop the support completely from the next release. But that is not required.
Good idea, Done.
integration-tests/modules/legacy_nft_assetnft_test.go line 123 at r2 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
You use
nft.MsgSendbut how about use thecosmosnft.MsgSendas well ? And same for the queries.
cosmosnft.MsgSend is tested in the other file assetnft_test.go here we specifically test the legacy handlers.
integration-tests/upgrade/nft_migration_test.go line 96 at r2 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
IMO in the after we should test both cosmos and legacy API to be sure that the data is still accesable with the legacy API after the migration.
We do just a single test here to make sure that the data is correctly migrated. the behavior of the APIs is tested in integration tests and unit tests.
x/asset/nft/keeper/keeper.go line 137 at r2 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
Why don't we validate it anymore?
wojtek asked the same.
the nft module has remove the validation criteria for class id. we have this check here, so our generated id will not clash with the validation of the nft module. since nft module is not doing validation, this check is not needed.
x/asset/nft/types/token.go line 115 at r2 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
Why don't we validate it anymore?
nft id validation was part of the nft module and it is now removed, so the only validation that must be enforced is the one introduced by assetnft module.
There was a problem hiding this comment.
Reviewable status: 51 of 53 files reviewed, all discussions resolved (waiting on @wojtek-coreum and @ysv)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ysv)
Description
Earlier version of Coreum used Cosmos SDK v0.45 which did not have nft module, so we backported that module from v0.46. Now that we have moved to Cosmos SDK v0.47, we can migrate to the native nft module inside the SDK. This PR achieves just that.
Reviewers checklist:
Authors checklist
This change is