Problem: No module to do NFT transfers via IBC #860
Conversation
Codecov Report
@@ Coverage Diff @@
## master #860 +/- ##
==========================================
+ Coverage 15.50% 16.92% +1.42%
==========================================
Files 105 119 +14
Lines 15516 14367 -1149
==========================================
+ Hits 2405 2432 +27
+ Misses 12622 11441 -1181
- Partials 489 494 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
tomtau
requested review from
mmsqe,
yihuang,
adu-crypto,
damoncro,
JayT106,
leejw51crypto and
thomas-nguy
|
/runsim |
|
Simulation tests started and triggered by |
|
✅ |
There was a problem hiding this comment.
I had a quick read against https://github.com/cosmos/ibc/tree/main/spec/app/ics-721-nft-transfer and it looks all right; the integration test is also good that it checks that previously created NFTs will continue working post-upgrade.
Currently, I find three things missing:
- the test coverage is low. At least from what I can see, some of the spec logic doesn't appear to be covered, so it'll be good to add at least a few Keeper unit tests for it. (Off the top of my mind: covering those multi-hop-related logic A->B->C and going back, as there are always Else branches for the NFTs that weren't from the origin... and covering timeouts/refunds)
- doc/spec -- maybe it can be in a separate PR... mostly to point to a specific revision https://github.com/cosmos/ibc/tree/main/spec/app/ics-721-nft-transfer and update https://github.com/crypto-org-chain/chain-main/blob/master/doc/architecture/adr-004.md (I guess either a new ADR and supersede that one, or updates in it)
- app wiring for ICS29 -- I assume the NFT transfer won't be automatically covered by ICS29, so it may need extra code for the middleware... also not sure how complex -- maybe better in a separate PR.
otherwise it seems good to go, but ideally it's good to get a second pair of eyes to look at it, given it's not a trivial PR (@yihuang @adu-crypto @damoncro @JayT106 @leejw51crypto @thomas-nguy @mmsqe)
| ) | ||
|
|
||
| assert rsp["uri"] == tokenuri, rsp | ||
| assert rsp["owner"] == addr_src, rsp |
There was a problem hiding this comment.
adding some tests which ensure bad arguments not working.
such as
very long length tokenid, denomid?
some invalid data
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Added integration tests for multi-hop nft transfer |
Current implementation already wires ICS29 with nft-transfer module |
|
are those number conversion warnings false positives? |
Yes. One is converting 10 minutes to nanoseconds and storing it in |
Added timeout test |
devashishdxt
requested review from
tomtau,
mmsqe,
JayT106,
adu-crypto and
leejw51crypto
|
/runsim |
|
Simulation tests started and triggered by |
|
✅ |
Solution: Add nft-transfer module. Note: Have to include ibc-go git submodule for building protobuf files. Can be removed once this issue is closed: cosmos/ibc-go#1345
Solution: Add nft-transfer module. Note: Have to include ibc-go git submodule for building protobuf files. Can be removed once this issue is closed: cosmos/ibc-go#1345
Solution: Add
nft-transfermodule.Note: Have to include
ibc-gogit submodule for building protobuf files. Can be removed once this issue is closed: cosmos/ibc-go#1345