NFT transfer over IBC #2407
NFT transfer over IBC #2407
Conversation
crates/core/src/types/ibc.rs
Outdated
| /// Returns the trace path and the token string if the trace is an NFT one | ||
| pub fn is_nft_trace( | ||
| trace: impl AsRef<str>, | ||
| ) -> Option<(NftTracePath, String, String)> { |
There was a problem hiding this comment.
| ) -> Option<(NftTracePath, String, String)> { | |
| ) -> Option<(NftTracePath, BaseClass, TokenId)> { |
I would add some kind of domain specific type here, but given we're pressed for time feel free to ignore this comment.
| @@ -51,20 +53,18 @@ where | |||
| // Convert IBC amount to Namada amount for the token | |||
| let denom = read_denom(&*self.inner.borrow(), &token) | |||
| .map_err(ContextError::from)? | |||
| .unwrap_or(token::Denomination(0)); | |||
| .unwrap_or(Denomination(0)); | |||
There was a problem hiding this comment.
honestly, I think we should always assume a denomination of 0 during protocol execution. denomination should be a concern of clients. for the protocol, amounts should already be handed in correctly denominated.
this will likely always return 0 anyway, unless we specifically store the denomination of an ibc token
for the purposes of this review, I'd leave things as is, especially because we have very little time, but I would consult this code later and change things accordingly.
crates/sdk/src/masp.rs
Outdated
| @@ -735,15 +737,11 @@ impl<U: ShieldedUtils + MaybeSend + MaybeSync> ShieldedContext<U> { | |||
| .block | |||
| .data; | |||
|
|
|||
| for (idx, tx_event) in txs_results { | |||
| for idx in txs_results { | |||
There was a problem hiding this comment.
This looks good to me, unfortunately we modified this file in the last release to require tx events for other things but ibc (basically to use the changed keys instead of Transfer) so we'll probably need to revert some of your changes here when merging
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2407 +/- ##
==========================================
+ Coverage 53.88% 54.00% +0.12%
==========================================
Files 308 310 +2
Lines 100154 101492 +1338
==========================================
+ Hits 53967 54814 +847
- Misses 46187 46678 +491 ☔ View full report in Codecov by Sentry. |
* origin/yuji/ibc-nft: for clippy remove stale comments not handle masp when receiving fails refactoring add changelog fmt fix to get memo for NFT transfer fix benches fix test fix tests update ibc-rs update ibc-rs not delete NFT metadata nft transfer option data back to upstream branch update ibc-rs and add some tests add nft send test add nft transfer WIP: impl context for new ibc-rs
* origin/yuji/ibc-nft: for clippy remove stale comments not handle masp when receiving fails refactoring add changelog fmt fix to get memo for NFT transfer fix benches fix test fix tests update ibc-rs update ibc-rs not delete NFT metadata nft transfer option data back to upstream branch update ibc-rs and add some tests add nft send test add nft transfer WIP: impl context for new ibc-rs
* origin/yuji/ibc-nft: for clippy remove stale comments not handle masp when receiving fails refactoring add changelog fmt fix to get memo for NFT transfer fix benches fix test fix tests update ibc-rs update ibc-rs not delete NFT metadata nft transfer option data back to upstream branch update ibc-rs and add some tests add nft send test add nft transfer WIP: impl context for new ibc-rs
Describe your changes
Need to wait for ibc-rs upstream for ICS-721 implementationibc-rstov0.50.0including ICS-721 impl.ibc-rs' NFT transfer module/contextMsgTransferandMsgNftTransferibc-rs'MsgTransfer/MsgNftTransferandOption<IbcShieldedTransfer>DenominatedAmountwithAmountin IBC handling (Also fixedtx_prelude::token::transfer)Indicate on which release or other PRs this topic is based on
v0.31.9Checklist before merging to
draft