From fb52da8e3d928ad92ef3afddebd007d6ebc6a79e Mon Sep 17 00:00:00 2001 From: mr-t Date: Thu, 25 Jan 2024 22:40:52 +0100 Subject: [PATCH 01/14] test backtransfer to banned recipient, but also to to regular recpient --- contracts/cw721-tester/src/lib.rs | 32 ++++++++++-------- ts-relayer-tests/src/ics721.spec.ts | 51 +++++++++++++++++++++++++---- 2 files changed, 63 insertions(+), 20 deletions(-) diff --git a/contracts/cw721-tester/src/lib.rs b/contracts/cw721-tester/src/lib.rs index ed686b1e..dbbfc474 100644 --- a/contracts/cw721-tester/src/lib.rs +++ b/contracts/cw721-tester/src/lib.rs @@ -1,7 +1,7 @@ use cosmwasm_schema::cw_serde; #[cfg(not(feature = "library"))] use cosmwasm_std::entry_point; -use cosmwasm_std::{Addr, Binary, Deps, DepsMut, Empty, Env, MessageInfo, Response, StdResult}; +use cosmwasm_std::{Binary, Deps, DepsMut, Empty, Env, MessageInfo, Response, StdResult}; use cw2::set_contract_version; use cw721_base::{msg, ContractError, Extension}; use cw_storage_plus::Item; @@ -14,13 +14,13 @@ pub struct InstantiateMsg { pub name: String, pub symbol: String, pub minter: String, - /// An address which will be unable to transfer NFTs away from - /// themselves (they are a black hole). If this address attempts a - /// `TransferNft` message it will fail with an out-of-gas error. - pub target: String, + /// An address which will be unable receive NFT on `TransferNft` message + /// If `TransferNft` message attempts sending to banned recipient + /// it will fail with an out-of-gas error. + pub banned_recipient: String, } -const TARGET: Item = Item::new("target"); +const BANNED_RECIPIENT: Item = Item::new("banned_recipient"); const CONTRACT_NAME: &str = "crates.io:cw721-gas-tester"; const CONTRACT_VERSION: &str = env!("CARGO_PKG_VERSION"); @@ -43,7 +43,7 @@ pub fn instantiate( withdraw_address: None, }, )?; - TARGET.save(deps.storage, &deps.api.addr_validate(&msg.target)?)?; + BANNED_RECIPIENT.save(deps.storage, &msg.banned_recipient)?; set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?; Ok(response) @@ -56,13 +56,17 @@ pub fn execute( info: MessageInfo, msg: ExecuteMsg, ) -> Result { - if matches!(msg, ExecuteMsg::TransferNft { .. }) && info.sender == TARGET.load(deps.storage)? { - // loop here causes the relayer to hang while it tries to - // simulate the TX. - panic!("gotem") - // loop {} - } else { - cw721_base::entry::execute(deps, env, info, msg) + match msg.clone() { + ExecuteMsg::TransferNft { recipient, .. } => { + if recipient == BANNED_RECIPIENT.load(deps.storage)? { + // loop here causes the relayer to hang while it tries to + // simulate the TX. + panic!("gotem") + // loop {} + } + cw721_base::entry::execute(deps, env, info, msg) + } + _ => cw721_base::entry::execute(deps, env, info, msg), } } diff --git a/ts-relayer-tests/src/ics721.spec.ts b/ts-relayer-tests/src/ics721.spec.ts index 530de734..4514dc03 100644 --- a/ts-relayer-tests/src/ics721.spec.ts +++ b/ts-relayer-tests/src/ics721.spec.ts @@ -386,9 +386,9 @@ test.serial( // Verify we got an error assertAckErrors(info.acksFromA); - // assert NFT on chain A is returned to owner + // assert NFT on chain B is returned to owner tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); - t.is(osmoClient.senderAddress, tokenOwner.owner); + t.is(osmoAddr, tokenOwner.owner); t.log(`NFT #${tokenId} returned to owner`); } ); @@ -415,7 +415,7 @@ test.serial("malicious NFT", async (t) => { name: "evil", symbol: "evil", minter: wasmClient.senderAddress, - target: wasmIcs721, // panic every time the ICS721 contract tries to return a NFT. + banned_recipient: "banned_recipient", // panic every time the ICS721 contract tries to transfer NFT to this address }, }, }); @@ -452,7 +452,7 @@ test.serial("malicious NFT", async (t) => { assertAckSuccess(info.acksFromB); - t.log("transferring back to wasm chain"); + t.log("transferring back to wasm chain to banned recipient"); const osmoClassId = `${t.context.channel.channel.dest.portId}/${t.context.channel.channel.dest.channelId}/${cw721}`; const osmoCw721 = await osmoClient.sign.queryContractSmart(osmoIcs721, { @@ -460,7 +460,7 @@ test.serial("malicious NFT", async (t) => { }); ibcMsg = { - receiver: wasmAddr, + receiver: "banned_recipient", channel_id: channel.channel.dest.channelId, timeout: { block: { @@ -481,10 +481,49 @@ test.serial("malicious NFT", async (t) => { t.log("relaying packets"); - const pending = await channel.link.getPendingPackets("B"); + let pending = await channel.link.getPendingPackets("B"); t.is(pending.length, 1); // Despite the transfer panicking, a fail ack should be returned. info = await channel.link.relayAll(); assertAckErrors(info.acksFromA); + // assert NFT on chain B is returned to owner + let tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); + t.is(osmoAddr, tokenOwner.owner); + t.log(`NFT #${tokenId} returned to owner`); + + t.log("transferring back to wasm chain to recipient", wasmAddr); + + ibcMsg = { + receiver: wasmAddr, + channel_id: channel.channel.dest.channelId, + timeout: { + block: { + revision: 1, + height: 90000, + }, + }, + }; + + transferResponse = await sendNft( + osmoClient, + osmoCw721, + osmoCw721OutgoingProxy, + ibcMsg, + tokenId + ); + t.truthy(transferResponse); + + t.log("relaying packets"); + + pending = await channel.link.getPendingPackets("B"); + t.is(pending.length, 1); + + // Verify we got a success + info = await channel.link.relayAll(); + assertAckSuccess(info.acksFromB); + + // assert NFT on chain A is returned to owner + tokenOwner = await ownerOf(wasmClient, cw721, tokenId); + t.is(wasmAddr, tokenOwner.owner); }); From be897b811ed2e97bda42e1e9cfe39c1cf6ba2f41 Mon Sep 17 00:00:00 2001 From: mr-t Date: Fri, 26 Jan 2024 14:39:00 +0100 Subject: [PATCH 02/14] e2e ts relayer test: - happy path on back transfer - edge case: transfer via unknown channel --- ts-relayer-tests/src/ics721.spec.ts | 331 ++++++++++++++++------------ ts-relayer-tests/src/utils.ts | 6 +- 2 files changed, 189 insertions(+), 148 deletions(-) diff --git a/ts-relayer-tests/src/ics721.spec.ts b/ts-relayer-tests/src/ics721.spec.ts index 4514dc03..ca3a6087 100644 --- a/ts-relayer-tests/src/ics721.spec.ts +++ b/ts-relayer-tests/src/ics721.spec.ts @@ -29,13 +29,15 @@ interface TestContext { wasmCw721: string; wasmIcs721: string; wasmCw721IncomingProxy: string; + wasmCw721OutgoingProxy: string; osmoCw721: string; osmoIcs721: string; + osmoCw721IncomingProxy: string; osmoCw721OutgoingProxy: string; channel: ChannelAndLinkInfo; - + onlyOsmoIncomingChannel: ChannelAndLinkInfo; // this channel is WLed only in incoming proxy on osmo side otherChannel: ChannelAndLinkInfo; } @@ -70,6 +72,10 @@ const standardSetup = async (t: ExecutionContext) => { path: WASM_FILE_CW721_INCOMING_PROXY, instantiateMsg: undefined, }, + cw721OutgoingProxy: { + path: WASM_FILE_CW721_OUTGOING_PROXY, + instantiateMsg: undefined, + }, ics721: { path: WASM_FILE_CW_ICS721_ICS721, instantiateMsg: undefined, @@ -84,6 +90,10 @@ const standardSetup = async (t: ExecutionContext) => { minter: osmoClient.senderAddress, }, }, + cw721IncomingProxy: { + path: WASM_FILE_CW721_INCOMING_PROXY, + instantiateMsg: undefined, + }, cw721OutgoingProxy: { path: WASM_FILE_CW721_OUTGOING_PROXY, instantiateMsg: undefined, @@ -106,10 +116,14 @@ const standardSetup = async (t: ExecutionContext) => { const wasmCw721IncomingProxyId = info.wasmContractInfos.cw721IncomingProxy.codeId; + const osmoCw721IncomingProxyId = + info.osmoContractInfos.cw721IncomingProxy.codeId; const wasmIcs721Id = info.wasmContractInfos.ics721.codeId; const osmoIcs721Id = info.osmoContractInfos.ics721.codeId; + const wasmCw721OutgoingProxyId = + info.wasmContractInfos.cw721OutgoingProxy.codeId; const osmoCw721OutgoingProxyId = info.osmoContractInfos.cw721OutgoingProxy.codeId; @@ -147,11 +161,17 @@ const standardSetup = async (t: ExecutionContext) => { Order.ORDER_UNORDERED, "ics721-1" ); - t.log(`- channel: ${JSON.stringify(channelInfo, bigIntReplacer, 2)}`); + t.log( + `- channel for incoming proxy on both chains: ${JSON.stringify( + channelInfo.channel, + bigIntReplacer, + 2 + )}` + ); t.context.channel = channelInfo; t.log( - `instantiating wasm cw721-incoming-proxy (${wasmCw721IncomingProxyId}) for channel ${channelInfo.channel.src.channelId}` + `instantiating wasm cw721-incoming-proxy (${wasmCw721IncomingProxyId}) with channel ${channelInfo.channel.src.channelId}` ); const { contractAddress: wasmCw721IncomingProxy } = await instantiateContract( wasmClient, @@ -166,11 +186,61 @@ const standardSetup = async (t: ExecutionContext) => { t.context.wasmCw721IncomingProxy = wasmCw721IncomingProxy; t.log( - `migrate ${wasmIcs721} contract to use incoming proxy ${wasmCw721IncomingProxy}` + `migrate ${wasmIcs721} contract with incoming proxy ${wasmCw721IncomingProxy}` ); await migrate(wasmClient, wasmIcs721, wasmIcs721Id, wasmCw721IncomingProxy); + const onlyOsmoIncomingChannelInfo = await createIbcConnectionAndChannel( + wasmClient, + osmoClient, + wasmIcs721, + osmoIcs721, + Order.ORDER_UNORDERED, + "ics721-1" + ); + t.log( + `- channel for incoming proxy only on wasm chain: ${JSON.stringify( + onlyOsmoIncomingChannelInfo.channel, + bigIntReplacer, + 2 + )}` + ); + t.context.onlyOsmoIncomingChannel = onlyOsmoIncomingChannelInfo; + + t.log( + `instantiating osmo cw721-incoming-proxy (${osmoCw721IncomingProxyId}) with channel ${channelInfo.channel.dest.channelId}and ${onlyOsmoIncomingChannelInfo.channel.dest.channelId}` + ); + const { contractAddress: osmoCw721IncomingProxy } = await instantiateContract( + osmoClient, + osmoCw721IncomingProxyId, + { + origin: osmoIcs721, + channels: [ + channelInfo.channel.dest.channelId, + onlyOsmoIncomingChannelInfo.channel.dest.channelId, + ], + }, + "label incoming proxy" + ); + t.log(`- osmo cw721-incoming-proxy address: ${osmoCw721IncomingProxy}`); + t.context.osmoCw721IncomingProxy = osmoCw721IncomingProxy; + const per_block = 10; // use high rate limit to avoid test failures + t.log( + `instantiating wasm cw721-outgoing-proxy (${wasmCw721OutgoingProxyId}) with ${per_block} per blocks rate limit` + ); + const { contractAddress: wasmCw721OutgoingProxy } = await instantiateContract( + wasmClient, + wasmCw721OutgoingProxyId, + { + origin: wasmIcs721, + rate_limit: { per_block }, + }, + "label outgoing proxy" + ); + t.log(`- wasm cw721-outgoing-proxy address: ${wasmCw721OutgoingProxy}`); + t.context.wasmCw721OutgoingProxy = wasmCw721OutgoingProxy; + t.log( `instantiating osmo cw721-outgoing-proxy (${osmoCw721OutgoingProxyId}) with ${per_block} per blocks rate limit` ); @@ -187,13 +257,24 @@ const standardSetup = async (t: ExecutionContext) => { t.context.osmoCw721OutgoingProxy = osmoCw721OutgoingProxy; t.log( - `migrate ${osmoIcs721} contract to use outgoing proxy ${osmoCw721OutgoingProxy}` + `migrate ${wasmIcs721} contract with incoming (${wasmCw721IncomingProxy}) and outgoing proxy (${wasmCw721OutgoingProxy})` + ); + await migrate( + wasmClient, + wasmIcs721, + wasmIcs721Id, + wasmCw721IncomingProxy, + wasmCw721OutgoingProxy + ); + + t.log( + `migrate ${osmoIcs721} contract with incoming (${osmoCw721IncomingProxy}) and outgoing proxy (${osmoCw721OutgoingProxy})` ); await migrate( osmoClient, osmoIcs721, osmoIcs721Id, - undefined, + osmoCw721IncomingProxy, osmoCw721OutgoingProxy ); @@ -208,6 +289,13 @@ const standardSetup = async (t: ExecutionContext) => { Order.ORDER_UNORDERED, "ics721-1" ); + t.log( + `- other channel not WLed for incoming proxy: ${JSON.stringify( + otherChannelInfo.channel, + bigIntReplacer, + 2 + )}` + ); t.context.otherChannel = otherChannelInfo; t.pass(); @@ -221,20 +309,24 @@ test.serial("transfer NFT: wasmd -> osmo", async (t) => { wasmAddr, wasmCw721, wasmIcs721, + wasmCw721OutgoingProxy, osmoClient, osmoAddr, osmoIcs721, channel, + otherChannel, } = t.context; - t.log(JSON.stringify(wasmClient, undefined, 2)); - const tokenId = "1"; + let tokenId = "1"; await mint(wasmClient, wasmCw721, tokenId, wasmAddr, undefined); // assert token is minted let tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); t.is(wasmAddr, tokenOwner.owner); - const ibcMsg = { + // ==== happy path: transfer NFT to osmo chain and back to wasm chain ==== + // test transfer NFT to osmo chain + t.log(`transfering to osmo chain via ${channel.channel.src.channelId}`); + let ibcMsg = { receiver: osmoAddr, channel_id: channel.channel.src.channelId, timeout: { @@ -244,65 +336,38 @@ test.serial("transfer NFT: wasmd -> osmo", async (t) => { }, }, }; - - t.log(`transfering to osmo chain via ${channel.channel.src.channelId}`); - - const transferResponse = await sendNft( + let transferResponse = await sendNft( wasmClient, wasmCw721, - wasmIcs721, + wasmCw721OutgoingProxy, ibcMsg, tokenId ); t.truthy(transferResponse); + // Relay and verify we got a success t.log("relaying packets"); - - const info = await channel.link.relayAll(); - - // Verify we got a success - assertAckSuccess(info.acksFromB); + let info = await channel.link.relayAll(); + assertAckSuccess(info.acksFromA); // assert NFT on chain A is locked/owned by ICS contract tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); t.is(wasmIcs721, tokenOwner.owner); - - const osmoClassId = `${t.context.channel.channel.dest.portId}/${t.context.channel.channel.dest.channelId}/${t.context.wasmCw721}`; + // assert NFT on chain B is owned by osmoAddr + const osmoClassId = `${channel.channel.dest.portId}/${channel.channel.dest.channelId}/${t.context.wasmCw721}`; const osmoCw721 = await osmoClient.sign.queryContractSmart(osmoIcs721, { nft_contract: { class_id: osmoClassId }, }); - tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); t.is(osmoAddr, tokenOwner.owner); -}); -test.serial( - "transfer NFT with osmo outgoing and wasm incoming proxy", - async (t) => { - await standardSetup(t); - - const { - wasmClient, - wasmAddr, - wasmIcs721, - osmoClient, - osmoAddr, - osmoCw721, - osmoIcs721, - osmoCw721OutgoingProxy, - channel, - otherChannel, - } = t.context; - - // test 1: transfer via outgoing proxy and using WLed channel by incoming proxy - let tokenId = "1"; - t.log(`transferring NFT #${tokenId} from osmo to wasmd chain`); - await mint(osmoClient, osmoCw721, tokenId, osmoAddr, undefined); - // assert token is minted - let tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); - t.is(osmoAddr, tokenOwner.owner); - - let ibcMsg = { + // test back transfer NFT to wasm chain + t.log(`transfering back to wasm chain via ${channel.channel.dest.channelId}`); + transferResponse = await sendNft( + osmoClient, + osmoCw721, + t.context.osmoCw721OutgoingProxy, + { receiver: wasmAddr, channel_id: channel.channel.dest.channelId, timeout: { @@ -311,95 +376,69 @@ test.serial( height: 90000, }, }, - }; - - t.log( - `transfering to wasm chain via ${channel.channel.dest.channelId} and outgoing proxy ${osmoCw721OutgoingProxy}` - ); - - let transferResponse = await sendNft( - osmoClient, - osmoCw721, - osmoCw721OutgoingProxy, - ibcMsg, - tokenId - ); - t.truthy(transferResponse); - - t.log("relaying packets"); - - let info = await channel.link.relayAll(); - - // Verify we got a success - assertAckSuccess(info.acksFromA); - - // assert NFT on chain A is locked/owned by ICS contract - tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); - t.is(osmoIcs721, tokenOwner.owner); - t.log(`NFT #${tokenId} locked by ICS721 contract`); - - const wasmClassId = `${t.context.channel.channel.src.portId}/${t.context.channel.channel.src.channelId}/${t.context.osmoCw721}`; - const wasmCw721 = await wasmClient.sign.queryContractSmart(wasmIcs721, { - nft_contract: { class_id: wasmClassId }, - }); - - tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); - t.is(wasmAddr, tokenOwner.owner); - t.log(`NFT #${tokenId} transferred to ${wasmAddr}`); - - // test 2: transfer via outgoing proxy and using unknown channel by incoming proxy - tokenId = "2"; - t.log(`transferring NFT #${tokenId} from osmo to wasmd chain`); - await mint(osmoClient, osmoCw721, tokenId, osmoAddr, undefined); - // assert token is minted - tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); - t.is(osmoAddr, tokenOwner.owner); - - ibcMsg = { - receiver: wasmAddr, - channel_id: otherChannel.channel.dest.channelId, - timeout: { - block: { - revision: 1, - height: 90000, - }, - }, - }; + }, + tokenId + ); + t.truthy(transferResponse); + t.log("relaying packets"); - t.log( - `transfering to wasm chain via ${otherChannel.channel.dest.channelId}` - ); + // Verify we got a success + info = await channel.link.relayAll(); + assertAckSuccess(info.acksFromA); - transferResponse = await sendNft( - osmoClient, - osmoCw721, - osmoCw721OutgoingProxy, - ibcMsg, - tokenId - ); - t.truthy(transferResponse); + // assert NFT on chain A is returned to owner + tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); + t.is(wasmAddr, tokenOwner.owner); - t.log("relaying packets"); + // ==== test transfer NFT to osmo chain via unknown, not WLed channel by incoming proxy ==== + // test rejected NFT transfer due to unknown channel by incoming proxy + tokenId = "2"; + await mint(wasmClient, wasmCw721, tokenId, wasmAddr, undefined); + // assert token is minted + tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); + t.is(wasmAddr, tokenOwner.owner); - info = await otherChannel.link.relayAll(); + t.log( + `transfering to osmo chain via unknown ${otherChannel.channel.src.channelId}` + ); + ibcMsg = { + receiver: osmoAddr, + channel_id: otherChannel.channel.src.channelId, + timeout: { + block: { + revision: 1, + height: 90000, + }, + }, + }; + transferResponse = await sendNft( + wasmClient, + wasmCw721, + wasmCw721OutgoingProxy, + ibcMsg, + tokenId + ); + t.truthy(transferResponse); - // Verify we got an error - assertAckErrors(info.acksFromA); + // Relay and verify we got an error + t.log("relaying packets"); + info = await otherChannel.link.relayAll(); + assertAckErrors(info.acksFromA); - // assert NFT on chain B is returned to owner - tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); - t.is(osmoAddr, tokenOwner.owner); - t.log(`NFT #${tokenId} returned to owner`); - } -); + // assert NFT on chain A is returned to owner + tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); + t.is(wasmAddr, tokenOwner.owner); + + // ==== test transfer NFT to osmo chain via channel WLed ONLY osmo incoming proxy and back to wasm chain ==== +}); test.serial("malicious NFT", async (t) => { await standardSetup(t); - const { wasmClient, wasmAddr, wasmIcs721, + wasmCw721OutgoingProxy, osmoClient, osmoAddr, osmoIcs721, @@ -408,6 +447,7 @@ test.serial("malicious NFT", async (t) => { } = t.context; const tokenId = "1"; + // instantiate malicious cw721 contract const res = await uploadAndInstantiate(wasmClient, { cw721_gas_tester: { path: MALICIOUS_CW721, @@ -415,15 +455,15 @@ test.serial("malicious NFT", async (t) => { name: "evil", symbol: "evil", minter: wasmClient.senderAddress, - banned_recipient: "banned_recipient", // panic every time the ICS721 contract tries to transfer NFT to this address + banned_recipient: "banned_recipient", // panic every time, on back transfer, when ICS721 tries to transfer/unescrow NFT to this address }, }, }); - const cw721 = res.cw721_gas_tester.address as string; + // ==== test malicious NFT transfer to osmo chain ==== await mint(wasmClient, cw721, tokenId, wasmAddr, undefined); - + t.log("transferring to osmo chain"); let ibcMsg = { receiver: osmoAddr, channel_id: channel.channel.src.channelId, @@ -434,31 +474,32 @@ test.serial("malicious NFT", async (t) => { }, }, }; - - t.log("transferring to osmo chain"); - let transferResponse = await sendNft( wasmClient, cw721, - wasmIcs721, + wasmCw721OutgoingProxy, ibcMsg, tokenId ); t.truthy(transferResponse); t.log("relaying packets"); - let info = await channel.link.relayAll(); - assertAckSuccess(info.acksFromB); - t.log("transferring back to wasm chain to banned recipient"); - + // assert NFT on chain A is locked/owned by ICS contract + let tokenOwner = await ownerOf(wasmClient, cw721, tokenId); + t.is(wasmIcs721, tokenOwner.owner); + // assert NFT on chain B is owned by osmoAddr const osmoClassId = `${t.context.channel.channel.dest.portId}/${t.context.channel.channel.dest.channelId}/${cw721}`; const osmoCw721 = await osmoClient.sign.queryContractSmart(osmoIcs721, { nft_contract: { class_id: osmoClassId }, }); + tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); + t.is(osmoAddr, tokenOwner.owner); + // ==== test malicious NFT back transfer to banned recipient on wasm chain ==== + t.log("transferring back to wasm chain to banned recipient"); ibcMsg = { receiver: "banned_recipient", channel_id: channel.channel.dest.channelId, @@ -469,7 +510,6 @@ test.serial("malicious NFT", async (t) => { }, }, }; - transferResponse = await sendNft( osmoClient, osmoCw721, @@ -478,22 +518,23 @@ test.serial("malicious NFT", async (t) => { tokenId ); t.truthy(transferResponse); + // before relay NFT escrowed by ICS721 + tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); + t.is(osmoIcs721, tokenOwner.owner); t.log("relaying packets"); - let pending = await channel.link.getPendingPackets("B"); t.is(pending.length, 1); - // Despite the transfer panicking, a fail ack should be returned. info = await channel.link.relayAll(); assertAckErrors(info.acksFromA); - // assert NFT on chain B is returned to owner - let tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); + // assert after failed relay, NFT on chain B is returned to owner + tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); t.is(osmoAddr, tokenOwner.owner); t.log(`NFT #${tokenId} returned to owner`); + // ==== test malicious NFT transfer to regular recipient wasm chain ==== t.log("transferring back to wasm chain to recipient", wasmAddr); - ibcMsg = { receiver: wasmAddr, channel_id: channel.channel.dest.channelId, @@ -514,12 +555,10 @@ test.serial("malicious NFT", async (t) => { ); t.truthy(transferResponse); + // Relay and verify we got a success t.log("relaying packets"); - pending = await channel.link.getPendingPackets("B"); t.is(pending.length, 1); - - // Verify we got a success info = await channel.link.relayAll(); assertAckSuccess(info.acksFromB); diff --git a/ts-relayer-tests/src/utils.ts b/ts-relayer-tests/src/utils.ts index d965be95..06ba5696 100644 --- a/ts-relayer-tests/src/utils.ts +++ b/ts-relayer-tests/src/utils.ts @@ -232,10 +232,12 @@ export function assertAckErrors(acks: AckWithMetadata[]) { for (const ack of acks) { const parsed = JSON.parse(fromUtf8(ack.acknowledgement)); if (parsed.result) { - throw new Error(`Ack result unexpectedly set`); + throw new Error(`Ack result unexpectedly set: ${JSON.stringify(parsed)}`); } if (!parsed.error) { - throw new Error(`Ack error unexpectedly empty`); + throw new Error( + `Ack error unexpectedly empty: ${JSON.stringify(parsed)}` + ); } } } From ae6c7d3a7eef19bc4b75f66c4e26e47f00b53f2e Mon Sep 17 00:00:00 2001 From: mr-t Date: Fri, 26 Jan 2024 21:55:58 +0100 Subject: [PATCH 03/14] fix back transfer, remove entries in outgoing channel only in case all sub messages have succesfully completed --- packages/ics721/schema/ics721.json | 25 +++ packages/ics721/schema/raw/execute.json | 25 +++ packages/ics721/src/execute.rs | 14 ++ packages/ics721/src/ibc_packet_receive.rs | 32 ++- packages/ics721/src/msg.rs | 2 + ts-relayer-tests/src/ics721-utils.ts | 41 ++++ ts-relayer-tests/src/ics721.spec.ts | 231 +++++++++++++++++++++- 7 files changed, 360 insertions(+), 10 deletions(-) diff --git a/packages/ics721/schema/ics721.json b/packages/ics721/schema/ics721.json index bc19702d..d5b0577a 100644 --- a/packages/ics721/schema/ics721.json +++ b/packages/ics721/schema/ics721.json @@ -245,6 +245,31 @@ }, "additionalProperties": false }, + { + "type": "object", + "required": [ + "redeem_outgoing_channel_entries" + ], + "properties": { + "redeem_outgoing_channel_entries": { + "type": "array", + "items": { + "type": "array", + "items": [ + { + "$ref": "#/definitions/ClassId" + }, + { + "$ref": "#/definitions/TokenId" + } + ], + "maxItems": 2, + "minItems": 2 + } + } + }, + "additionalProperties": false + }, { "description": "Mints a NFT of collection class_id for receiver with the provided id and metadata. Only callable by this contract.", "type": "object", diff --git a/packages/ics721/schema/raw/execute.json b/packages/ics721/schema/raw/execute.json index 19cdd78d..0f266cab 100644 --- a/packages/ics721/schema/raw/execute.json +++ b/packages/ics721/schema/raw/execute.json @@ -112,6 +112,31 @@ }, "additionalProperties": false }, + { + "type": "object", + "required": [ + "redeem_outgoing_channel_entries" + ], + "properties": { + "redeem_outgoing_channel_entries": { + "type": "array", + "items": { + "type": "array", + "items": [ + { + "$ref": "#/definitions/ClassId" + }, + { + "$ref": "#/definitions/TokenId" + } + ], + "maxItems": 2, + "minItems": 2 + } + } + }, + "additionalProperties": false + }, { "description": "Mints a NFT of collection class_id for receiver with the provided id and metadata. Only callable by this contract.", "type": "object", diff --git a/packages/ics721/src/execute.rs b/packages/ics721/src/execute.rs index 4c46ada9..e060f6f5 100644 --- a/packages/ics721/src/execute.rs +++ b/packages/ics721/src/execute.rs @@ -294,6 +294,9 @@ where tokens, receiver, } => self.callback_mint(deps, class_id, tokens, receiver), + CallbackMsg::RedeemOutgoingChannelEntries(entries) => { + self.callback_redeem_outgoing_channel_entries(deps, entries) + } CallbackMsg::Conjunction { operands } => { Ok(Response::default().add_messages(operands)) @@ -475,6 +478,17 @@ where .add_messages(mint)) } + fn callback_redeem_outgoing_channel_entries( + &self, + deps: DepsMut, + entries: Vec<(ClassId, TokenId)>, + ) -> Result, ContractError> { + for (class_id, token_id) in entries { + OUTGOING_CLASS_TOKEN_TO_CHANNEL.remove(deps.storage, (class_id, token_id)); + } + Ok(Response::default().add_attribute("method", "callback_redeem_outgoing_channel_entries")) + } + fn migrate( &self, deps: DepsMut, diff --git a/packages/ics721/src/ibc_packet_receive.rs b/packages/ics721/src/ibc_packet_receive.rs index 02951694..164baf50 100644 --- a/packages/ics721/src/ibc_packet_receive.rs +++ b/packages/ics721/src/ibc_packet_receive.rs @@ -94,7 +94,6 @@ pub(crate) fn receive_ibc_packet( // We previously sent this NFT out on this // channel. Unlock the local version for the // receiver. - OUTGOING_CLASS_TOKEN_TO_CHANNEL.remove(deps.storage, key); messages.push(Action::Redemption { token_id, class_id: local_class_id, @@ -317,9 +316,23 @@ impl ActionAggregator { } // we can only have redeem or create, not both - if let Some(redeem) = self.redemption { - m.push(redeem.into_wasm_msg(contract.clone(), receiver.to_string())?) - } + let outgoing_class_tokens: Option> = + if let Some(redeem) = self.redemption { + m.push( + redeem + .clone() + .into_wasm_msg(contract.clone(), receiver.to_string())?, + ); + let (class_id, token_ids) = (redeem.class.id, redeem.token_ids); + Some( + token_ids + .into_iter() + .map(|token_id| (class_id.clone(), token_id)) + .collect(), + ) + } else { + None + }; if let Some(create) = self.creation { m.push(create.into_wasm_msg(contract.clone(), receiver.into_string())?) } @@ -327,6 +340,17 @@ impl ActionAggregator { if let Some(callback_msg) = callback_msg { m.push(callback_msg) } + + // once all other submessages are done, we can redeem entries in the outgoing channel + if let Some(outgoing_class_tokens) = outgoing_class_tokens { + m.push(WasmMsg::Execute { + contract_addr: contract.to_string(), + msg: to_json_binary(&ExecuteMsg::Callback( + CallbackMsg::RedeemOutgoingChannelEntries(outgoing_class_tokens), + ))?, + funds: vec![], + }); + } let message = if m.len() == 1 { m[0].clone() } else { diff --git a/packages/ics721/src/msg.rs b/packages/ics721/src/msg.rs index c7a902cc..43d410d8 100644 --- a/packages/ics721/src/msg.rs +++ b/packages/ics721/src/msg.rs @@ -63,6 +63,8 @@ pub enum CallbackMsg { /// Information about the vouchers been redeemed. redeem: VoucherRedemption, }, + /// Redeem all entries in outgoing channel. + RedeemOutgoingChannelEntries(Vec<(ClassId, TokenId)>), /// Mints a NFT of collection class_id for receiver with the /// provided id and metadata. Only callable by this contract. Mint { diff --git a/ts-relayer-tests/src/ics721-utils.ts b/ts-relayer-tests/src/ics721-utils.ts index 398eefac..a380de98 100644 --- a/ts-relayer-tests/src/ics721-utils.ts +++ b/ts-relayer-tests/src/ics721-utils.ts @@ -21,3 +21,44 @@ export function migrate( undefined ); } + +export function migrateIncomingProxy( + client: CosmWasmSigner, + contractAddress: string, + codeId: number, + channels?: string[], + origin?: string +) { + const msg = { + with_update: { origin, channels }, + }; + return client.sign.migrate( + client.senderAddress, + contractAddress, + codeId, + msg, + "auto", + undefined + ); +} + +// ######### query +export function nftContracts( + client: CosmWasmSigner, + contractAddress: string +): Promise<[string, string][]> { + const msg = { + nft_contracts: {}, + }; + return client.sign.queryContractSmart(contractAddress, msg); +} + +export function outgoingChannels( + client: CosmWasmSigner, + contractAddress: string +): Promise<[[string, string], string][]> { + const msg = { + outgoing_channels: {}, + }; + return client.sign.queryContractSmart(contractAddress, msg); +} diff --git a/ts-relayer-tests/src/ics721.spec.ts b/ts-relayer-tests/src/ics721.spec.ts index ca3a6087..c3525934 100644 --- a/ts-relayer-tests/src/ics721.spec.ts +++ b/ts-relayer-tests/src/ics721.spec.ts @@ -1,10 +1,16 @@ import { CosmWasmSigner } from "@confio/relayer"; +import { fromUtf8 } from "@cosmjs/encoding"; import anyTest, { ExecutionContext, TestFn } from "ava"; import { Order } from "cosmjs-types/ibc/core/channel/v1/channel"; import { instantiateContract } from "./controller"; -import { mint, ownerOf, sendNft } from "./cw721-utils"; -import { migrate } from "./ics721-utils"; +import { allTokens, mint, ownerOf, sendNft } from "./cw721-utils"; +import { + migrate, + migrateIncomingProxy, + nftContracts, + outgoingChannels, +} from "./ics721-utils"; import { assertAckErrors, assertAckSuccess, @@ -28,6 +34,7 @@ interface TestContext { wasmCw721: string; wasmIcs721: string; + wasmCw721IncomingProxyId: number; wasmCw721IncomingProxy: string; wasmCw721OutgoingProxy: string; @@ -116,6 +123,7 @@ const standardSetup = async (t: ExecutionContext) => { const wasmCw721IncomingProxyId = info.wasmContractInfos.cw721IncomingProxy.codeId; + t.context.wasmCw721IncomingProxyId = wasmCw721IncomingProxyId; const osmoCw721IncomingProxyId = info.osmoContractInfos.cw721IncomingProxy.codeId; @@ -309,12 +317,15 @@ test.serial("transfer NFT: wasmd -> osmo", async (t) => { wasmAddr, wasmCw721, wasmIcs721, + wasmCw721IncomingProxyId, + wasmCw721IncomingProxy, wasmCw721OutgoingProxy, osmoClient, osmoAddr, osmoIcs721, channel, otherChannel, + onlyOsmoIncomingChannel, } = t.context; let tokenId = "1"; @@ -353,11 +364,14 @@ test.serial("transfer NFT: wasmd -> osmo", async (t) => { // assert NFT on chain A is locked/owned by ICS contract tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); t.is(wasmIcs721, tokenOwner.owner); - // assert NFT on chain B is owned by osmoAddr - const osmoClassId = `${channel.channel.dest.portId}/${channel.channel.dest.channelId}/${t.context.wasmCw721}`; - const osmoCw721 = await osmoClient.sign.queryContractSmart(osmoIcs721, { + // assert NFT minted on chain B + let osmoClassId = `${channel.channel.dest.portId}/${channel.channel.dest.channelId}/${t.context.wasmCw721}`; + let osmoCw721 = await osmoClient.sign.queryContractSmart(osmoIcs721, { nft_contract: { class_id: osmoClassId }, }); + let allNFTs = await allTokens(osmoClient, osmoCw721); + t.true(allNFTs.tokens.length === 1); + // assert NFT on chain B is owned by osmoAddr tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); t.is(osmoAddr, tokenOwner.owner); @@ -386,6 +400,9 @@ test.serial("transfer NFT: wasmd -> osmo", async (t) => { info = await channel.link.relayAll(); assertAckSuccess(info.acksFromA); + // assert NFT burned on chain B + allNFTs = await allTokens(osmoClient, osmoCw721); + t.true(allNFTs.tokens.length === 0); // assert NFT on chain A is returned to owner tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); t.is(wasmAddr, tokenOwner.owner); @@ -429,7 +446,209 @@ test.serial("transfer NFT: wasmd -> osmo", async (t) => { tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); t.is(wasmAddr, tokenOwner.owner); - // ==== test transfer NFT to osmo chain via channel WLed ONLY osmo incoming proxy and back to wasm chain ==== + // ==== test transfer NFT to osmo chain via channel WLed ONLY on osmo incoming proxy and back to wasm chain ==== + tokenId = "3"; + await mint(wasmClient, wasmCw721, tokenId, wasmAddr, undefined); + // assert token is minted + tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); + t.is(wasmAddr, tokenOwner.owner); + + // test transfer NFT to osmo chain + t.log( + `transfering to osmo chain via ${onlyOsmoIncomingChannel.channel.src.channelId}` + ); + ibcMsg = { + receiver: osmoAddr, + channel_id: onlyOsmoIncomingChannel.channel.src.channelId, + timeout: { + block: { + revision: 1, + height: 90000, + }, + }, + }; + transferResponse = await sendNft( + wasmClient, + wasmCw721, + wasmCw721OutgoingProxy, + ibcMsg, + tokenId + ); + t.truthy(transferResponse); + + // Relay and verify we got a success + t.log("relaying packets"); + info = await onlyOsmoIncomingChannel.link.relayAll(); + assertAckSuccess(info.acksFromA); + + // assert 1 entry for outgoing channels + let wasmOutgoingClassTokenToChannelList = await outgoingChannels( + wasmClient, + wasmIcs721 + ); + t.log( + `- outgoing channels: ${JSON.stringify( + wasmOutgoingClassTokenToChannelList + )}` + ); + t.true( + wasmOutgoingClassTokenToChannelList.length === 1, + `outgoing channels must have one entry: ${JSON.stringify( + wasmOutgoingClassTokenToChannelList + )}` + ); + + // assert NFT minted on chain B + osmoClassId = `${onlyOsmoIncomingChannel.channel.dest.portId}/${onlyOsmoIncomingChannel.channel.dest.channelId}/${t.context.wasmCw721}`; + osmoCw721 = await osmoClient.sign.queryContractSmart(osmoIcs721, { + nft_contract: { class_id: osmoClassId }, + }); + allNFTs = await allTokens(osmoClient, osmoCw721); + t.true(allNFTs.tokens.length === 1); + // assert NFT on chain B is owned by osmoAddr + tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); + t.is(osmoAddr, tokenOwner.owner); + // assert NFT on chain A is locked/owned by ICS contract + tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); + t.is(wasmIcs721, tokenOwner.owner); + // assert NFT on chain B is owned by osmoAddr + osmoClassId = `${onlyOsmoIncomingChannel.channel.dest.portId}/${onlyOsmoIncomingChannel.channel.dest.channelId}/${t.context.wasmCw721}`; + osmoCw721 = await osmoClient.sign.queryContractSmart(osmoIcs721, { + nft_contract: { class_id: osmoClassId }, + }); + tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); + t.is(osmoAddr, tokenOwner.owner); + + // test back transfer NFT to wasm chain, where onlyOsmoIncomingChannel is not WLed on wasm chain + t.log( + `transfering back to wasm chain via unknown ${onlyOsmoIncomingChannel.channel.dest.channelId}` + ); + transferResponse = await sendNft( + osmoClient, + osmoCw721, + t.context.osmoCw721OutgoingProxy, + { + receiver: wasmAddr, + channel_id: onlyOsmoIncomingChannel.channel.dest.channelId, + timeout: { + block: { + revision: 1, + height: 90000, + }, + }, + }, + tokenId + ); + t.truthy(transferResponse); + // before relay NFT escrowed by ICS721 + tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); + t.is(osmoIcs721, tokenOwner.owner); + + // Relay and verify we got an error + t.log("relaying packets"); + info = await onlyOsmoIncomingChannel.link.relayAll(); + for (const ack of info.acksFromB) { + const parsed = JSON.parse(fromUtf8(ack.acknowledgement)); + t.log(`- ack: ${JSON.stringify(parsed)}`); + } + assertAckErrors(info.acksFromB); + + // assert after failed relay, NFT on chain B is returned to owner + allNFTs = await allTokens(osmoClient, osmoCw721); + t.true(allNFTs.tokens.length === 1); + // assert NFT is returned to sender on osmo chain + tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); + t.is(osmoAddr, tokenOwner.owner); + + // ==== WL channel on wasm chain and test back transfer again ==== + t.log( + `migrate ${wasmCw721IncomingProxy} contract and add channel ${onlyOsmoIncomingChannel.channel.src.channelId}` + ); + await migrateIncomingProxy( + wasmClient, + wasmCw721IncomingProxy, + wasmCw721IncomingProxyId, + [ + channel.channel.src.channelId, + onlyOsmoIncomingChannel.channel.src.channelId, + ] + ); + + // test back transfer NFT to wasm chain, where onlyOsmoIncomingChannel is not WLed on wasm chain + t.log( + `transfering back to wasm chain via WLed ${onlyOsmoIncomingChannel.channel.dest.channelId}` + ); + transferResponse = await sendNft( + osmoClient, + osmoCw721, + t.context.osmoCw721OutgoingProxy, + { + receiver: wasmAddr, + channel_id: onlyOsmoIncomingChannel.channel.dest.channelId, + timeout: { + block: { + revision: 1, + height: 90000, + }, + }, + }, + tokenId + ); + t.truthy(transferResponse); + // before relay NFT escrowed by ICS721 + tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); + t.is(osmoIcs721, tokenOwner.owner); + + allNFTs = await allTokens(osmoClient, osmoCw721); + t.log(`- all tokens: ${JSON.stringify(allNFTs)}`); + + // query nft contracts + let nftContractsToClassIdList = await nftContracts(wasmClient, wasmIcs721); + t.log(`- nft contracts: ${JSON.stringify(nftContractsToClassIdList)}`); + t.true( + nftContractsToClassIdList.length === 1, + `nft contracts must have exactly one entry: ${JSON.stringify( + nftContractsToClassIdList + )}` + ); + + // Relay and verify success + t.log("relaying packets"); + info = await onlyOsmoIncomingChannel.link.relayAll(); + for (const ack of info.acksFromB) { + const parsed = JSON.parse(fromUtf8(ack.acknowledgement)); + t.log(`- ack: ${JSON.stringify(parsed)}`); + } + assertAckSuccess(info.acksFromB); + + // assert outgoing channels is empty + wasmOutgoingClassTokenToChannelList = await outgoingChannels( + wasmClient, + wasmIcs721 + ); + t.true( + wasmOutgoingClassTokenToChannelList.length === 0, + `outgoing channels not empty: ${JSON.stringify( + wasmOutgoingClassTokenToChannelList + )}` + ); + + // assert after success relay, NFT on chain B is burned + allNFTs = await allTokens(osmoClient, osmoCw721); + t.log(`- all tokens: ${JSON.stringify(allNFTs)}`); + t.true(allNFTs.tokens.length === 0); + // assert list is unchanged + nftContractsToClassIdList = await nftContracts(wasmClient, wasmIcs721); + t.log(`- nft contracts: ${JSON.stringify(nftContractsToClassIdList)}`); + t.true( + nftContractsToClassIdList.length === 1, + `nft contracts must have exactly one entry: ${JSON.stringify( + nftContractsToClassIdList + )}` + ); + // assert NFT is returned to sender on wasm chain + tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); + t.is(wasmAddr, tokenOwner.owner); }); test.serial("malicious NFT", async (t) => { From 2afbce72a92044b346223accd44b91ab8d419dba Mon Sep 17 00:00:00 2001 From: mr-t Date: Fri, 26 Jan 2024 23:24:49 +0100 Subject: [PATCH 04/14] rename --- packages/ics721/src/ibc_packet_receive.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ics721/src/ibc_packet_receive.rs b/packages/ics721/src/ibc_packet_receive.rs index 164baf50..f2b985c8 100644 --- a/packages/ics721/src/ibc_packet_receive.rs +++ b/packages/ics721/src/ibc_packet_receive.rs @@ -316,7 +316,7 @@ impl ActionAggregator { } // we can only have redeem or create, not both - let outgoing_class_tokens: Option> = + let redeem_outgoing_class_tokens: Option> = if let Some(redeem) = self.redemption { m.push( redeem @@ -342,7 +342,7 @@ impl ActionAggregator { } // once all other submessages are done, we can redeem entries in the outgoing channel - if let Some(outgoing_class_tokens) = outgoing_class_tokens { + if let Some(outgoing_class_tokens) = redeem_outgoing_class_tokens { m.push(WasmMsg::Execute { contract_addr: contract.to_string(), msg: to_json_binary(&ExecuteMsg::Callback( From 6357673ece1f64f88b1caddf4aff6f9ac3933bac Mon Sep 17 00:00:00 2001 From: mr-t Date: Sat, 27 Jan 2024 19:55:09 +0100 Subject: [PATCH 05/14] simplify receive_ibc_packet(), create sub message directly, remove action aggregator --- packages/ics721/src/ibc_packet_receive.rs | 344 ++++++++-------------- 1 file changed, 118 insertions(+), 226 deletions(-) diff --git a/packages/ics721/src/ibc_packet_receive.rs b/packages/ics721/src/ibc_packet_receive.rs index f2b985c8..eba932c5 100644 --- a/packages/ics721/src/ibc_packet_receive.rs +++ b/packages/ics721/src/ibc_packet_receive.rs @@ -1,6 +1,6 @@ use cosmwasm_std::{ - from_json, to_json_binary, Addr, Binary, DepsMut, Empty, Env, IbcPacket, IbcReceiveResponse, - StdResult, SubMsg, WasmMsg, + from_json, to_json_binary, Addr, DepsMut, Empty, Env, IbcPacket, IbcReceiveResponse, StdResult, + SubMsg, WasmMsg, }; use sha2::{Digest, Sha256}; use zip_optional::Zippable; @@ -25,34 +25,6 @@ use ics721_types::{ token_types::{Class, ClassId, Token, TokenId}, }; -/// Every incoming token has some associated action. -enum Action { - /// Debt-voucher redemption. - Redemption { - class_id: ClassId, - token_id: TokenId, - }, - /// Debt-voucher creation. - Creation { class_id: ClassId, token: Token }, -} - -/// Internal type for aggregating actions. Actions can be added via -/// `add_action`. Once aggregation has completed, a -/// `HandlePacketReceive` submessage can be created via the -/// `into_submessage` method. -/// -/// Unlike `class_id`, class data and uri will always be the same -/// across one transfer so we store only one copy at the top level and -/// initialize it at creation time. -#[derive(Default)] -struct ActionAggregator { - class_uri: Option, - class_data: Option, - - redemption: Option, - creation: Option, -} - pub(crate) fn receive_ibc_packet( deps: DepsMut, env: Env, @@ -69,18 +41,27 @@ pub(crate) fn receive_ibc_packet( // Check if NFT is local if not get the local class id let maybe_local_class_id = try_pop_source_prefix(&packet.src, &data.class_id); let callback = get_receive_callback(&data); + // If class is not local, its something new + let local_class_id = if let Some(local_class_id) = maybe_local_class_id { + ClassId::new(local_class_id) + } else { + let local_prefix = get_endpoint_prefix(&packet.dest); + ClassId::new(format!("{}{}", local_prefix, data.class_id)) + }; - let action_aggregator = data + let redemption_or_create = data .token_ids .into_iter() .zip_optional(data.token_uris) .zip_optional(data.token_data) .try_fold( - Vec::::with_capacity(token_count), - |mut messages, ((token_id, token_uri), token_data)| -> StdResult<_> { + ( + Vec::::with_capacity(token_count), + Vec::::with_capacity(token_count), + ), + |mut redemption_or_create, ((token_id, token_uri), token_data)| -> StdResult<_> { // If class is not local, its something new - if let Some(local_class_id) = maybe_local_class_id { - let local_class_id = ClassId::new(local_class_id); + if maybe_local_class_id.is_some() { let key: (ClassId, TokenId) = (local_class_id.clone(), token_id.clone()); let outgoing_channel = OUTGOING_CLASS_TOKEN_TO_CHANNEL.may_load(deps.storage, key.clone())?; @@ -94,11 +75,8 @@ pub(crate) fn receive_ibc_packet( // We previously sent this NFT out on this // channel. Unlock the local version for the // receiver. - messages.push(Action::Redemption { - token_id, - class_id: local_class_id, - }); - return Ok(messages); + redemption_or_create.0.push(token_id); + return Ok(redemption_or_create); } } // It's not something we've sent out before => make a @@ -111,39 +89,39 @@ pub(crate) fn receive_ibc_packet( (local_class_id.clone(), token_id.clone()), &packet.dest.channel_id, )?; - messages.push(Action::Creation { - class_id: local_class_id, - token: Token { - id: token_id, - uri: token_uri, - data: token_data, - }, + redemption_or_create.1.push(Token { + id: token_id, + uri: token_uri, + data: token_data, }); - Ok(messages) + Ok(redemption_or_create) }, - )? - .into_iter() - .fold( - ActionAggregator::new(data.class_uri, data.class_data), - ActionAggregator::add_action, - ); + )?; - // All token ids in the transfer must be either a redeption or creation - // they can't be both, if they are both something is wrong. - if action_aggregator.redemption.is_some() && action_aggregator.creation.is_some() { + let is_redemption = if !redemption_or_create.0.is_empty() && !redemption_or_create.1.is_empty() + { + // All token ids in the transfer must be either a redeption or creation + // they can't be both, if they are both something is wrong. return Err(ContractError::InvalidTransferBothActions); - } + } else if !redemption_or_create.0.is_empty() { + true + } else if !redemption_or_create.1.is_empty() { + false + } else { + // This should never happen, as we must have at least 1 of the above actions + return Err(ContractError::InvalidTransferNoAction); + }; // if there is a callback, generate the callback message let callback_msg = if let Some((receive_callback_data, receive_callback_addr)) = callback { // callback require the nft contract, get it using the class id from the action - let nft_contract = if let Some(voucher) = action_aggregator.redemption.clone() { + let nft_contract = if is_redemption { // If its a redemption, it means we already have the contract address in storage CLASS_ID_TO_NFT_CONTRACT - .load(deps.storage, voucher.class.id.clone()) - .map_err(|_| ContractError::NoNftContractForClassId(voucher.class.id.to_string())) - } else if let Some(voucher) = action_aggregator.creation.clone() { + .load(deps.storage, local_class_id.clone()) + .map_err(|_| ContractError::NoNftContractForClassId(local_class_id.to_string())) + } else { // If its a creation action, we can use the instantiate2 function to get the nft contract // we don't care of the contract is instantiated yet or not, as later submessage will instantiate it if its not. // The reason we use instantiate2 here is because we don't know if it was already instantiated or not. @@ -153,7 +131,7 @@ pub(crate) fn receive_ibc_packet( // - using class id as salt for instantiating nft contract guarantees a) predictable address and b) uniqueness // for this salt must be of length 32 bytes, so we use sha256 to hash class id let mut hasher = Sha256::new(); - hasher.update(voucher.class.id.as_bytes()); + hasher.update(local_class_id.as_bytes()); let salt = hasher.finalize().to_vec(); get_instantiate2_address( @@ -162,9 +140,6 @@ pub(crate) fn receive_ibc_packet( &salt, cw721_code_id, ) - } else { - // This should never happen, as we must have at least 1 of the above actions - Err(ContractError::InvalidTransferNoAction) }?; generate_receive_callback_msg( @@ -180,10 +155,50 @@ pub(crate) fn receive_ibc_packet( let incoming_proxy_msg = get_incoming_proxy_msg(deps.storage, packet.clone(), cloned_data.clone())?; - let submessage = action_aggregator.into_submessage( + let voucher_message = match is_redemption { + true => { + let redemption = VoucherRedemption { + class: Class { + id: local_class_id.clone(), + uri: data.class_uri.clone(), + data: data.class_data.clone(), + }, + token_ids: redemption_or_create.0, + }; + let redeem_outgoing_class_tokens: Option> = Some( + redemption + .token_ids + .clone() + .into_iter() + .map(|token_id| (local_class_id.clone(), token_id)) + .collect(), + ); + ( + redemption.into_wasm_msg(env.contract.address.clone(), receiver.to_string())?, + redeem_outgoing_class_tokens, + ) + } + false => { + let creation = VoucherCreation { + class: Class { + id: local_class_id.clone(), + uri: data.class_uri.clone(), + data: data.class_data.clone(), + }, + tokens: redemption_or_create.1, + }; + ( + creation.into_wasm_msg(env.contract.address.clone(), receiver.to_string())?, + None, + ) + } + }; + + let submessage = into_submessage( env.contract.address, - receiver, + voucher_message.0, callback_msg, + voucher_message.1, incoming_proxy_msg, )?; @@ -201,167 +216,44 @@ pub(crate) fn receive_ibc_packet( .add_attribute("counterparty_channel", packet.src.channel_id)) } -impl ActionAggregator { - pub fn new(class_uri: Option, class_data: Option) -> Self { - Self { - class_uri, - class_data, - redemption: None, - creation: None, - } - } - - // the ics-721 rx logic is a functional implementation of this - // imperative pseudocode: - // - // ``` - // def select_actions(class_id, token, ibc_channel): - // (local_class_id, could_be_local) = pop_src_prefix(class_id) - // actions = [] - // - // for token in tokens: - // if could_be_local: - // returning_to_source = outgoing_tokens.has(token) - // if returning_to_source: - // outgoing_tokens.remove(token) - // actions.push(redeem_voucher, token, local_class_id) - // continue - // incoming_tokens.save(token) - // prefixed_class_id = prefix(class_id, ibc_channel) - // actions.push(create_voucher, token, prefixed_class_id) - // - // return actions - // ``` - // - // as `class_id` is fixed: - // - // 1. all `create_voucher` actions will have class id - // `prefixed_class_id` - // 2. all `redeem_voucher` actions will have class id - // `local_class_id` - // - // in other words: - // - // 1. `create_voucher` actions will all have the same `class_id` - // 2. `redeem_voucher` actions will all have the same `class_id` - // - // we make use of these properties here in that we only store one - // copy of class information per voucher action. - // - // --- - // - // tangental but nonetheless important aside: - // - // 3. not all create and redeem actions will have the same - // `class_id`. - // - // by counterexample: two identical tokens are sent by a malicious - // counterparty, the first removes the token from the - // outgoing_tokens map, the second then creates a create_voucher - // action. - // - // see `TestDoubleSendInSingleMessage` in `/e2e/adversarial_test.go` - // for a test demonstrating this. - // - // Having both redemption and creation action in the same transfer - // tells us its a malicious act that we should reject. - pub fn add_action(mut self, action: Action) -> Self { - match action { - Action::Redemption { class_id, token_id } => { - self.redemption = match self.redemption { - Some(mut r) => { - r.token_ids.push(token_id); - Some(r) - } - None => Some(VoucherRedemption { - class: Class { - id: class_id, - uri: self.class_uri.clone(), - data: self.class_data.clone(), - }, - token_ids: vec![token_id], - }), - } - } - Action::Creation { class_id, token } => { - self.creation = match self.creation { - Some(mut c) => { - c.tokens.push(token); - Some(c) - } - None => Some(VoucherCreation { - class: Class { - id: class_id, - uri: self.class_uri.clone(), - data: self.class_data.clone(), - }, - tokens: vec![token], - }), - } - } - }; - self +pub fn into_submessage( + contract: Addr, + voucher_message: WasmMsg, + callback_msg: Option, + redeem_outgoing_class_tokens: Option>, + incoming_proxy_msg: Option, +) -> StdResult> { + let mut m = Vec::with_capacity(3); // 3 is the max number of submessages we can have + if let Some(incoming_proxy_msg) = incoming_proxy_msg { + m.push(incoming_proxy_msg) } - pub fn into_submessage( - self, - contract: Addr, - receiver: Addr, - callback_msg: Option, - incoming_proxy_msg: Option, - ) -> StdResult> { - let mut m = Vec::with_capacity(3); // 3 is the max number of submessages we can have - if let Some(incoming_proxy_msg) = incoming_proxy_msg { - m.push(incoming_proxy_msg) - } - - // we can only have redeem or create, not both - let redeem_outgoing_class_tokens: Option> = - if let Some(redeem) = self.redemption { - m.push( - redeem - .clone() - .into_wasm_msg(contract.clone(), receiver.to_string())?, - ); - let (class_id, token_ids) = (redeem.class.id, redeem.token_ids); - Some( - token_ids - .into_iter() - .map(|token_id| (class_id.clone(), token_id)) - .collect(), - ) - } else { - None - }; - if let Some(create) = self.creation { - m.push(create.into_wasm_msg(contract.clone(), receiver.into_string())?) - } + m.push(voucher_message); - if let Some(callback_msg) = callback_msg { - m.push(callback_msg) - } + if let Some(callback_msg) = callback_msg { + m.push(callback_msg) + } - // once all other submessages are done, we can redeem entries in the outgoing channel - if let Some(outgoing_class_tokens) = redeem_outgoing_class_tokens { - m.push(WasmMsg::Execute { - contract_addr: contract.to_string(), - msg: to_json_binary(&ExecuteMsg::Callback( - CallbackMsg::RedeemOutgoingChannelEntries(outgoing_class_tokens), - ))?, - funds: vec![], - }); - } - let message = if m.len() == 1 { - m[0].clone() - } else { - WasmMsg::Execute { - contract_addr: contract.into_string(), - msg: to_json_binary(&ExecuteMsg::Callback(CallbackMsg::Conjunction { - operands: m, - }))?, - funds: vec![], - } - }; - Ok(SubMsg::reply_always(message, ACK_AND_DO_NOTHING_REPLY_ID)) + // once all other submessages are done, we can redeem entries in the outgoing channel + if let Some(outgoing_class_tokens) = redeem_outgoing_class_tokens { + m.push(WasmMsg::Execute { + contract_addr: contract.to_string(), + msg: to_json_binary(&ExecuteMsg::Callback( + CallbackMsg::RedeemOutgoingChannelEntries(outgoing_class_tokens), + ))?, + funds: vec![], + }); } + let message = if m.len() == 1 { + m[0].clone() + } else { + WasmMsg::Execute { + contract_addr: contract.into_string(), + msg: to_json_binary(&ExecuteMsg::Callback(CallbackMsg::Conjunction { + operands: m, + }))?, + funds: vec![], + } + }; + Ok(SubMsg::reply_always(message, ACK_AND_DO_NOTHING_REPLY_ID)) } From f903de4aa81621a0f7590af2cfae6dc7cba67cb8 Mon Sep 17 00:00:00 2001 From: mr-t Date: Sat, 27 Jan 2024 23:54:10 +0100 Subject: [PATCH 06/14] fix move to sub message for saving incoming channel entries --- ics721_bug.md | 77 ++++++++++++ packages/ics721/src/execute.rs | 20 +++- packages/ics721/src/ibc_packet_receive.rs | 99 ++++++++-------- packages/ics721/src/msg.rs | 2 + packages/ics721/src/testing/ibc_tests.rs | 110 +++++++++++------- .../ics721/src/testing/integration_tests.rs | 1 - ts-relayer-tests/src/ics721-utils.ts | 10 ++ ts-relayer-tests/src/ics721.spec.ts | 93 +++++++++++++++ 8 files changed, 319 insertions(+), 93 deletions(-) create mode 100644 ics721_bug.md diff --git a/ics721_bug.md b/ics721_bug.md new file mode 100644 index 00000000..46619f72 --- /dev/null +++ b/ics721_bug.md @@ -0,0 +1,77 @@ +Bug in ICS721 when an NFT gets transferred back from chain B to A. + +Preliminary: NFT (forward) transferred from chain A to B +- NFT outcoume: NFT escrowed by ICS721 on chain A, NFT minted on newly, instantiated collection on chain B +- state changes: + - entry added on chain A: `OUTGOING_CLASS_TOKEN_TO_CHANNEL` (marker, check for identifying next time, NFT gets transferred back) + - entry added on chain B: `INCOMING_CLASS_TOKEN_TO_CHANNEL` + +Expected result on back transfer +- NFT burned on chain B +- "source/OG" NFT escrowed by ICS721 transferred to given recipent +- state changes: + - removed entry in `INCOMING_CLASS_TOKEN_TO_CHANNEL` on chain A + - removed entry in `OUTGOING_CLASS_TOKEN_TO_CHANNEL` on chain B +Actual result: +- NFT burned on chain B +- NFT minted on newly, instantiated collection +- NFT escrowed still escrowed by ICS721 on source collection + + + +By the spec of ics721, it is possible doing bulk transfers (whilst cw721 doesnt support this yet). +I believe this was the main objection of `ActionAggregator`'s design: +1. for each NFT either an `Action::Redemption` or `Action::Creation` is created: + - `Action::Redemption`: unescrow nft on forward transfer or + - `Action::Creation`: mint NFT and instantiate collection + - in addition for redemption/back transfer, entries in `OUTGOING_CLASS_TOKEN_TO_CHANNEL` are removed. That's the bug in contract's tx, we've identified +2. Each NFT action added to `ActionAggregator` + - interestingly `Action` enums are converted to either `VoucherRedemption` or `VoucherCreation` structs + - then converted structs are added to aggregator + - imo conversion not needed here, better approach here: +3. Finally all gets wrapped into a single sub message: + - create message list for each recreation or creation struct in aggregator: + - convert to WasmMsg with: + - `ExecuteMsg::Callback(CallbackMsg::CreateVouchers)` or + - `ExecuteMsg::Callback(CallbackMsg::RedeemVouchers)` + - message list represent a list of nft `operands` + - optional incoming proxy is added on top of operands list + - optional callback is added at the end of operands list + - merge message list into into single, final sub message + - final sub message is of type `reply all` for making sure TX always succeeds + - if there's only one message in list, this will be used for final sub message + - if message list contains more than one entries, they are merged into `ExecuteMsg::Callback(CallbackMsg::Conjunction {operands})` + +As a result a single `CallbackMsg` sub msg is created, which is either a +- `CallbackMsg::CreateVouchers` + - appends optional instantiate sub msg + - appends `CallbackMsg::Mint` msg +- `CallbackMsg::RedeemVouchers` + - transfer NFT to recipient +- `CallbackMsg::Conjunction` + - appends all to messages (operands callbacks (`CreateVouchers` or `RedeemVouchers` + optional incoming proxy + optional callback)) + +This guarantees: +- ics721 contract always succeeds +- each sub message handled serateley +- in case of sub msg failure + - it's partial state is reverted, but not its parent + - read more here: https://github.com/CosmWasm/cosmwasm/blob/main/SEMANTICS.md#submessages) + "... On an error, the subcall will revert any partial state changes due to this message, but not revert any state changes in the calling contract. The error may then be intercepted by the calling contract (for ReplyOn::Always and ReplyOn::Error). In this case, the messages error doesn't abort the whole transaction ..." + - remaining sub messages are not executed + - since operands in conjunction sub msg are added as messages (not sub msgs): + - sub msg is parent of operand messages + - ics721 is root parent of sub msg + - if any msg fails (eg. msg1: tranfer NFT1, msg2: tranfer NFT2) + - all messages and its parent/sub message is reverted + - root parent/ics721 is not reverted + +tl;dr: +- `receive_ibc_packet` response contains single callback sub msg, which: +- in case of failure, revert its own partial state, but wont revert contract's TX +- contains one or more messages + +The more I think about it, we can create this result response: +- create directly call back msgs +- hence, no intermediate step of Action enums and Action is required + diff --git a/packages/ics721/src/execute.rs b/packages/ics721/src/execute.rs index e060f6f5..7d6aeabd 100644 --- a/packages/ics721/src/execute.rs +++ b/packages/ics721/src/execute.rs @@ -20,8 +20,9 @@ use crate::{ msg::{CallbackMsg, ExecuteMsg, InstantiateMsg, MigrateMsg}, state::{ CollectionData, UniversalAllNftInfoResponse, ADMIN_USED_FOR_CW721, CLASS_ID_TO_CLASS, - CLASS_ID_TO_NFT_CONTRACT, CW721_CODE_ID, INCOMING_PROXY, NFT_CONTRACT_TO_CLASS_ID, - OUTGOING_CLASS_TOKEN_TO_CHANNEL, OUTGOING_PROXY, PO, TOKEN_METADATA, + CLASS_ID_TO_NFT_CONTRACT, CW721_CODE_ID, INCOMING_CLASS_TOKEN_TO_CHANNEL, INCOMING_PROXY, + NFT_CONTRACT_TO_CLASS_ID, OUTGOING_CLASS_TOKEN_TO_CHANNEL, OUTGOING_PROXY, PO, + TOKEN_METADATA, }, token_types::{VoucherCreation, VoucherRedemption}, ContractError, @@ -297,7 +298,9 @@ where CallbackMsg::RedeemOutgoingChannelEntries(entries) => { self.callback_redeem_outgoing_channel_entries(deps, entries) } - + CallbackMsg::AddIncomingChannelEntries(entries) => { + self.callback_save_incoming_channel_entries(deps, entries) + } CallbackMsg::Conjunction { operands } => { Ok(Response::default().add_messages(operands)) } @@ -489,6 +492,17 @@ where Ok(Response::default().add_attribute("method", "callback_redeem_outgoing_channel_entries")) } + fn callback_save_incoming_channel_entries( + &self, + deps: DepsMut, + entries: Vec<((ClassId, TokenId), String)>, + ) -> Result, ContractError> { + for (key, channel) in entries { + INCOMING_CLASS_TOKEN_TO_CHANNEL.save(deps.storage, key, &channel)?; + } + Ok(Response::default().add_attribute("method", "callback_redeem_outgoing_channel_entries")) + } + fn migrate( &self, deps: DepsMut, diff --git a/packages/ics721/src/ibc_packet_receive.rs b/packages/ics721/src/ibc_packet_receive.rs index eba932c5..7e136441 100644 --- a/packages/ics721/src/ibc_packet_receive.rs +++ b/packages/ics721/src/ibc_packet_receive.rs @@ -13,10 +13,7 @@ use crate::{ ibc::ACK_AND_DO_NOTHING_REPLY_ID, ibc_helpers::{get_endpoint_prefix, try_pop_source_prefix}, msg::{CallbackMsg, ExecuteMsg}, - state::{ - CLASS_ID_TO_NFT_CONTRACT, CW721_CODE_ID, INCOMING_CLASS_TOKEN_TO_CHANNEL, - OUTGOING_CLASS_TOKEN_TO_CHANNEL, PO, - }, + state::{CLASS_ID_TO_NFT_CONTRACT, CW721_CODE_ID, OUTGOING_CLASS_TOKEN_TO_CHANNEL, PO}, token_types::{VoucherCreation, VoucherRedemption}, ContractError, }; @@ -81,14 +78,6 @@ pub(crate) fn receive_ibc_packet( } // It's not something we've sent out before => make a // new NFT. - let local_prefix = get_endpoint_prefix(&packet.dest); - let local_class_id = ClassId::new(format!("{}{}", local_prefix, data.class_id)); - - INCOMING_CLASS_TOKEN_TO_CHANNEL.save( - deps.storage, - (local_class_id.clone(), token_id.clone()), - &packet.dest.channel_id, - )?; redemption_or_create.1.push(Token { id: token_id, uri: token_uri, @@ -155,7 +144,7 @@ pub(crate) fn receive_ibc_packet( let incoming_proxy_msg = get_incoming_proxy_msg(deps.storage, packet.clone(), cloned_data.clone())?; - let voucher_message = match is_redemption { + let voucher_and_channel_messages = match is_redemption { true => { let redemption = VoucherRedemption { class: Class { @@ -165,17 +154,22 @@ pub(crate) fn receive_ibc_packet( }, token_ids: redemption_or_create.0, }; - let redeem_outgoing_class_tokens: Option> = Some( - redemption - .token_ids - .clone() - .into_iter() - .map(|token_id| (local_class_id.clone(), token_id)) - .collect(), - ); + let redeem_outgoing_channels: Vec<(ClassId, TokenId)> = redemption + .token_ids + .clone() + .into_iter() + .map(|token_id| (local_class_id.clone(), token_id)) + .collect(); + let redeem_outgoing_channels_msg = WasmMsg::Execute { + contract_addr: env.contract.address.to_string(), + msg: to_json_binary(&ExecuteMsg::Callback( + CallbackMsg::RedeemOutgoingChannelEntries(redeem_outgoing_channels), + ))?, + funds: vec![], + }; ( redemption.into_wasm_msg(env.contract.address.clone(), receiver.to_string())?, - redeem_outgoing_class_tokens, + redeem_outgoing_channels_msg, ) } false => { @@ -187,18 +181,36 @@ pub(crate) fn receive_ibc_packet( }, tokens: redemption_or_create.1, }; + let add_incoming_channels: Vec<((ClassId, TokenId), String)> = creation + .tokens + .clone() + .into_iter() + .map(|token| { + ( + (local_class_id.clone(), token.id), + packet.dest.channel_id.clone(), + ) + }) + .collect(); + let add_incoming_channels_msg = WasmMsg::Execute { + contract_addr: env.contract.address.to_string(), + msg: to_json_binary(&ExecuteMsg::Callback( + CallbackMsg::AddIncomingChannelEntries(add_incoming_channels), + ))?, + funds: vec![], + }; ( creation.into_wasm_msg(env.contract.address.clone(), receiver.to_string())?, - None, + add_incoming_channels_msg, ) } }; let submessage = into_submessage( env.contract.address, - voucher_message.0, + voucher_and_channel_messages.0, + voucher_and_channel_messages.1, callback_msg, - voucher_message.1, incoming_proxy_msg, )?; @@ -219,41 +231,28 @@ pub(crate) fn receive_ibc_packet( pub fn into_submessage( contract: Addr, voucher_message: WasmMsg, + channel_message: WasmMsg, callback_msg: Option, - redeem_outgoing_class_tokens: Option>, incoming_proxy_msg: Option, ) -> StdResult> { - let mut m = Vec::with_capacity(3); // 3 is the max number of submessages we can have + let mut operands = Vec::with_capacity(3); // 3 is the max number of submessages we can have if let Some(incoming_proxy_msg) = incoming_proxy_msg { - m.push(incoming_proxy_msg) + operands.push(incoming_proxy_msg) } - m.push(voucher_message); + operands.push(voucher_message); if let Some(callback_msg) = callback_msg { - m.push(callback_msg) + operands.push(callback_msg) } - // once all other submessages are done, we can redeem entries in the outgoing channel - if let Some(outgoing_class_tokens) = redeem_outgoing_class_tokens { - m.push(WasmMsg::Execute { - contract_addr: contract.to_string(), - msg: to_json_binary(&ExecuteMsg::Callback( - CallbackMsg::RedeemOutgoingChannelEntries(outgoing_class_tokens), - ))?, - funds: vec![], - }); - } - let message = if m.len() == 1 { - m[0].clone() - } else { - WasmMsg::Execute { - contract_addr: contract.into_string(), - msg: to_json_binary(&ExecuteMsg::Callback(CallbackMsg::Conjunction { - operands: m, - }))?, - funds: vec![], - } + // once all other submessages are done, we can update incoming or outgoing channel + operands.push(channel_message); + + let message = WasmMsg::Execute { + contract_addr: contract.into_string(), + msg: to_json_binary(&ExecuteMsg::Callback(CallbackMsg::Conjunction { operands }))?, + funds: vec![], }; Ok(SubMsg::reply_always(message, ACK_AND_DO_NOTHING_REPLY_ID)) } diff --git a/packages/ics721/src/msg.rs b/packages/ics721/src/msg.rs index 43d410d8..b8832e0c 100644 --- a/packages/ics721/src/msg.rs +++ b/packages/ics721/src/msg.rs @@ -65,6 +65,8 @@ pub enum CallbackMsg { }, /// Redeem all entries in outgoing channel. RedeemOutgoingChannelEntries(Vec<(ClassId, TokenId)>), + /// Save all entries in incoming channel. + AddIncomingChannelEntries(Vec<((ClassId, TokenId), String)>), /// Mints a NFT of collection class_id for receiver with the /// provided id and metadata. Only callable by this contract. Mint { diff --git a/packages/ics721/src/testing/ibc_tests.rs b/packages/ics721/src/testing/ibc_tests.rs index b7c3ed55..cd3b7d84 100644 --- a/packages/ics721/src/testing/ibc_tests.rs +++ b/packages/ics721/src/testing/ibc_tests.rs @@ -1,11 +1,13 @@ +use core::panic; + use cosmwasm_schema::cw_serde; use cosmwasm_std::{ attr, from_json, testing::{mock_dependencies, mock_env, mock_info}, to_json_binary, to_json_vec, Addr, Attribute, Binary, DepsMut, Empty, Env, IbcAcknowledgement, IbcChannel, IbcChannelConnectMsg, IbcChannelOpenMsg, IbcEndpoint, IbcOrder, IbcPacket, - IbcPacketReceiveMsg, IbcTimeout, Order, Reply, Response, StdResult, SubMsgResponse, - SubMsgResult, Timestamp, WasmMsg, + IbcPacketReceiveMsg, IbcTimeout, Reply, Response, StdResult, SubMsgResponse, SubMsgResult, + Timestamp, WasmMsg, }; use crate::{ @@ -14,14 +16,14 @@ use crate::{ ibc_helpers::{ack_fail, ack_success, try_get_ack_error}, msg::{CallbackMsg, ExecuteMsg, InstantiateMsg, QueryMsg}, query::Ics721Query, - state::{CollectionData, INCOMING_CLASS_TOKEN_TO_CHANNEL, NFT_CONTRACT_TO_CLASS_ID, PO}, + state::{CollectionData, NFT_CONTRACT_TO_CLASS_ID, PO}, utils::get_collection_data, ContractError, }; use ics721_types::{ ibc_types::NonFungibleTokenPacketData, token_types::{ClassId, TokenId}, - types::Ics721Callbacks, + types::{Ics721Callbacks, ReceiverExecuteMsg}, }; const CONTRACT_PORT: &str = "wasm.address1"; @@ -447,32 +449,61 @@ fn test_ibc_packet_receive() { let mut deps = mock_dependencies(); let env = mock_env(); PO.set_pauser(&mut deps.storage, &deps.api, None).unwrap(); - Ics721Contract::default() + let response = Ics721Contract::default() .ibc_packet_receive(deps.as_mut(), env, packet) .unwrap(); + // assert there is only one message + assert_eq!(response.messages.len(), 1); - // check incoming classID and tokenID - let keys = INCOMING_CLASS_TOKEN_TO_CHANNEL - .keys(deps.as_mut().storage, None, None, Order::Ascending) - .collect::>>() - .unwrap(); - let class_id = format!( - "{}/{}/{}", - ibc_packet.dest.port_id, ibc_packet.dest.channel_id, "id" - ); - assert_eq!(keys, [(class_id, "1".to_string())]); - - // check channel - let key = ( - ClassId::new(keys[0].clone().0), - TokenId::new(keys[0].clone().1), - ); - assert_eq!( - INCOMING_CLASS_TOKEN_TO_CHANNEL - .load(deps.as_mut().storage, key) - .unwrap(), - ibc_packet.dest.channel_id, - ) + let conjunction_msg = match response.messages[0].msg.clone() { + cosmwasm_std::CosmosMsg::Wasm(WasmMsg::Execute { msg, .. }) => { + match from_json::(msg.clone()).unwrap() { + ExecuteMsg::Callback(callback_msg) => match callback_msg { + CallbackMsg::Conjunction { operands } => Some(operands), + _ => panic!("unexpected callback msg"), + }, + _ => panic!("unexpected execute msg"), + } + } + _ => panic!("unexpected cosmos msg"), + }; + assert!(conjunction_msg.is_some()); + + let operands = conjunction_msg.unwrap(); + assert_eq!(operands.len(), 2); + + let add_incoming_msg = operands[1].clone(); + match add_incoming_msg { + WasmMsg::Execute { msg, .. } => { + match from_json::(msg).ok() { + Some(msg) => match msg { + ExecuteMsg::Callback(msg) => match msg { + CallbackMsg::AddIncomingChannelEntries(class_token_to_channel_list) => { + let class_token_to_channel_list = class_token_to_channel_list + .into_iter() + .map(|((class, token), channel)| { + ((class.to_string(), token.into()), channel) + }) + .collect::>(); + // assert there is only one class token to channel entry + let class_id = format!( + "{}/{}/{}", + ibc_packet.dest.port_id, ibc_packet.dest.channel_id, "id" + ); + assert_eq!( + class_token_to_channel_list, + [((class_id, "1".to_string()), ibc_packet.dest.channel_id,)] + ); + } + _ => panic!("unexpected callback msg"), + }, + _ => panic!("unexpected execute msg"), + }, + _ => panic!("no callback msg"), + } + } + _ => panic!("unexpected wasm msg"), + } } #[test] @@ -705,20 +736,21 @@ fn test_different_memo_ignored() { .ibc_packet_receive(deps.as_mut(), env, packet) .unwrap(); - let memo_callback_msg = match res.messages[0].msg.clone() { - cosmwasm_std::CosmosMsg::Wasm(WasmMsg::Execute { msg, .. }) => { - match from_json::(msg).unwrap() { - ExecuteMsg::Callback(callback_msg) => match callback_msg { - CallbackMsg::Conjunction { operands } => Some(operands), - _ => Some(vec![]), - }, - _ => None, - } + if let cosmwasm_std::CosmosMsg::Wasm(WasmMsg::Execute { msg, .. }) = res.messages[0].msg.clone() + { + if let ExecuteMsg::Callback(CallbackMsg::Conjunction { operands }) = + from_json::(msg).unwrap() + { + // check each operand and make sure there is no memo callback + operands.into_iter().for_each(|operand| { + if let WasmMsg::Execute { msg, .. } = operand { + if let Ok(msg) = from_json::(msg) { + panic!("unexpected callback message: {:?}", msg) + } + } + }) } - _ => None, }; - assert!(memo_callback_msg.is_some()); - assert!(memo_callback_msg.unwrap().is_empty()); } #[test] diff --git a/packages/ics721/src/testing/integration_tests.rs b/packages/ics721/src/testing/integration_tests.rs index fa5cf077..7b292c15 100644 --- a/packages/ics721/src/testing/integration_tests.rs +++ b/packages/ics721/src/testing/integration_tests.rs @@ -2163,7 +2163,6 @@ fn test_migration() { assert_eq!(test.query_cw721_admin(), Some(admin),); // migrate without changing code id - println!(">>>>>>> migrate without changing code id"); test.app .execute( test.app.api().addr_make(ICS721_ADMIN_AND_PAUSER), diff --git a/ts-relayer-tests/src/ics721-utils.ts b/ts-relayer-tests/src/ics721-utils.ts index a380de98..9192c40b 100644 --- a/ts-relayer-tests/src/ics721-utils.ts +++ b/ts-relayer-tests/src/ics721-utils.ts @@ -62,3 +62,13 @@ export function outgoingChannels( }; return client.sign.queryContractSmart(contractAddress, msg); } + +export function incomingChannels( + client: CosmWasmSigner, + contractAddress: string +): Promise<[[string, string], string][]> { + const msg = { + incoming_channels: {}, + }; + return client.sign.queryContractSmart(contractAddress, msg); +} diff --git a/ts-relayer-tests/src/ics721.spec.ts b/ts-relayer-tests/src/ics721.spec.ts index c3525934..0068fe01 100644 --- a/ts-relayer-tests/src/ics721.spec.ts +++ b/ts-relayer-tests/src/ics721.spec.ts @@ -6,6 +6,7 @@ import { Order } from "cosmjs-types/ibc/core/channel/v1/channel"; import { instantiateContract } from "./controller"; import { allTokens, mint, ownerOf, sendNft } from "./cw721-utils"; import { + incomingChannels, migrate, migrateIncomingProxy, nftContracts, @@ -418,6 +419,31 @@ test.serial("transfer NFT: wasmd -> osmo", async (t) => { t.log( `transfering to osmo chain via unknown ${otherChannel.channel.src.channelId}` ); + const beforeWasmOutgoingClassTokenToChannelList = await outgoingChannels( + wasmClient, + wasmIcs721 + ); + const beforeWasmIncomingClassTokenToChannelList = await incomingChannels( + wasmClient, + wasmIcs721 + ); + const beforeWasmNftContractsToClassIdList = await nftContracts( + wasmClient, + wasmIcs721 + ); + const beforeOsmoOutgoingClassTokenToChannelList = await outgoingChannels( + osmoClient, + osmoIcs721 + ); + const beforeOsmoIncomingClassTokenToChannelList = await incomingChannels( + osmoClient, + osmoIcs721 + ); + const beforeOsmoNftContractsToClassIdList = await nftContracts( + osmoClient, + osmoIcs721 + ); + ibcMsg = { receiver: osmoAddr, channel_id: otherChannel.channel.src.channelId, @@ -441,6 +467,73 @@ test.serial("transfer NFT: wasmd -> osmo", async (t) => { t.log("relaying packets"); info = await otherChannel.link.relayAll(); assertAckErrors(info.acksFromA); + // assert no change before and after relay + const afterWasmOutgoingClassTokenToChannelList = await outgoingChannels( + wasmClient, + wasmIcs721 + ); + const afterWasmIncomingClassTokenToChannelList = await incomingChannels( + wasmClient, + wasmIcs721 + ); + const afterWasmNftContractsToClassIdList = await nftContracts( + wasmClient, + wasmIcs721 + ); + t.deepEqual( + beforeWasmOutgoingClassTokenToChannelList, + afterWasmOutgoingClassTokenToChannelList, + `outgoing channels must be unchanged: +- wasm before: ${JSON.stringify(beforeWasmOutgoingClassTokenToChannelList)} +- wasm after: ${JSON.stringify(afterWasmOutgoingClassTokenToChannelList)}` + ); + t.deepEqual( + beforeWasmIncomingClassTokenToChannelList, + afterWasmIncomingClassTokenToChannelList, + `incoming channels must be unchanged: +- wasm before: ${JSON.stringify(beforeWasmIncomingClassTokenToChannelList)} +- wasm after: ${JSON.stringify(afterWasmIncomingClassTokenToChannelList)}` + ); + t.deepEqual( + beforeWasmNftContractsToClassIdList, + afterWasmNftContractsToClassIdList, + `nft contracts must be unchanged: +- wasm before: ${JSON.stringify(beforeWasmNftContractsToClassIdList)} +- wasm after: ${JSON.stringify(afterWasmNftContractsToClassIdList)}` + ); + const afterOsmoOutgoingClassTokenToChannelList = await outgoingChannels( + osmoClient, + osmoIcs721 + ); + const afterOsmoIncomingClassTokenToChannelList = await incomingChannels( + osmoClient, + osmoIcs721 + ); + const afterOsmoNftContractsToClassIdList = await nftContracts( + osmoClient, + osmoIcs721 + ); + t.deepEqual( + beforeOsmoOutgoingClassTokenToChannelList, + afterOsmoOutgoingClassTokenToChannelList, + `outgoing channels must be unchanged: +- osmo before: ${JSON.stringify(beforeOsmoOutgoingClassTokenToChannelList)} +- osmo after: ${JSON.stringify(afterOsmoOutgoingClassTokenToChannelList)}` + ); + t.deepEqual( + beforeOsmoIncomingClassTokenToChannelList, + afterOsmoIncomingClassTokenToChannelList, + `incoming channels must be unchanged: +- osmo before: ${JSON.stringify(beforeOsmoIncomingClassTokenToChannelList)} +- osmo after: ${JSON.stringify(afterOsmoIncomingClassTokenToChannelList)}` + ); + t.deepEqual( + beforeOsmoNftContractsToClassIdList, + afterOsmoNftContractsToClassIdList, + `nft contracts must be unchanged: +- osmo before: ${JSON.stringify(beforeOsmoNftContractsToClassIdList)} +- osmo after: ${JSON.stringify(afterOsmoNftContractsToClassIdList)}` + ); // assert NFT on chain A is returned to owner tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); From f43f9c088fa0b4b69ff5cc2046eec08a0d5fbe8d Mon Sep 17 00:00:00 2001 From: mr-t Date: Sun, 28 Jan 2024 00:28:57 +0100 Subject: [PATCH 07/14] move to dedicated functions --- packages/ics721/src/ibc_packet_receive.rs | 167 +++++++++++++--------- 1 file changed, 99 insertions(+), 68 deletions(-) diff --git a/packages/ics721/src/ibc_packet_receive.rs b/packages/ics721/src/ibc_packet_receive.rs index 7e136441..07a95bac 100644 --- a/packages/ics721/src/ibc_packet_receive.rs +++ b/packages/ics721/src/ibc_packet_receive.rs @@ -1,6 +1,6 @@ use cosmwasm_std::{ - from_json, to_json_binary, Addr, DepsMut, Empty, Env, IbcPacket, IbcReceiveResponse, StdResult, - SubMsg, WasmMsg, + from_json, to_json_binary, Addr, Binary, Deps, DepsMut, Empty, Env, IbcPacket, + IbcReceiveResponse, StdResult, SubMsg, WasmMsg, }; use sha2::{Digest, Sha256}; use zip_optional::Zippable; @@ -31,10 +31,6 @@ pub(crate) fn receive_ibc_packet( let data: NonFungibleTokenPacketData = from_json(&packet.data)?; data.validate()?; - let cloned_data = data.clone(); - let receiver = deps.api.addr_validate(&data.receiver)?; - let token_count = data.token_ids.len(); - // Check if NFT is local if not get the local class id let maybe_local_class_id = try_pop_source_prefix(&packet.src, &data.class_id); let callback = get_receive_callback(&data); @@ -46,6 +42,59 @@ pub(crate) fn receive_ibc_packet( ClassId::new(format!("{}{}", local_prefix, data.class_id)) }; + let incoming_proxy_msg = + get_incoming_proxy_msg(deps.as_ref().storage, packet.clone(), data.clone())?; + + let (is_redemption, voucher_and_channel_messages) = create_voucher_and_channel_messages( + deps.as_ref(), + env.clone(), + data.clone(), + maybe_local_class_id, + local_class_id.clone(), + packet.clone(), + )?; + + // if there is a callback, generate the callback message + let callback_msg = create_callback_msg( + deps.as_ref(), + &env, + &data, + is_redemption, + callback, + local_class_id, + )?; + + let submessage = into_submessage( + env.contract.address, + voucher_and_channel_messages.0, + voucher_and_channel_messages.1, + callback_msg, + incoming_proxy_msg, + )?; + + let response = if let Some(memo) = data.memo { + IbcReceiveResponse::default().add_attribute("ics721_memo", memo) + } else { + IbcReceiveResponse::default() + }; + + Ok(response + .add_submessage(submessage) + .add_attribute("method", "receive_ibc_packet") + .add_attribute("class_id", data.class_id) + .add_attribute("local_channel", packet.dest.channel_id) + .add_attribute("counterparty_channel", packet.src.channel_id)) +} + +fn create_voucher_and_channel_messages( + deps: Deps, + env: Env, + data: NonFungibleTokenPacketData, + maybe_local_class_id: Option<&str>, + local_class_id: ClassId, + packet: IbcPacket, +) -> Result<(bool, (WasmMsg, WasmMsg)), ContractError> { + let token_count = data.token_ids.len(); let redemption_or_create = data .token_ids .into_iter() @@ -86,7 +135,6 @@ pub(crate) fn receive_ibc_packet( Ok(redemption_or_create) }, )?; - let is_redemption = if !redemption_or_create.0.is_empty() && !redemption_or_create.1.is_empty() { // All token ids in the transfer must be either a redeption or creation @@ -101,49 +149,7 @@ pub(crate) fn receive_ibc_packet( return Err(ContractError::InvalidTransferNoAction); }; - // if there is a callback, generate the callback message - let callback_msg = if let Some((receive_callback_data, receive_callback_addr)) = callback { - // callback require the nft contract, get it using the class id from the action - let nft_contract = if is_redemption { - // If its a redemption, it means we already have the contract address in storage - - CLASS_ID_TO_NFT_CONTRACT - .load(deps.storage, local_class_id.clone()) - .map_err(|_| ContractError::NoNftContractForClassId(local_class_id.to_string())) - } else { - // If its a creation action, we can use the instantiate2 function to get the nft contract - // we don't care of the contract is instantiated yet or not, as later submessage will instantiate it if its not. - // The reason we use instantiate2 here is because we don't know if it was already instantiated or not. - - let cw721_code_id = CW721_CODE_ID.load(deps.storage)?; - // for creating a predictable nft contract using, using instantiate2, we need: checksum, creator, and salt: - // - using class id as salt for instantiating nft contract guarantees a) predictable address and b) uniqueness - // for this salt must be of length 32 bytes, so we use sha256 to hash class id - let mut hasher = Sha256::new(); - hasher.update(local_class_id.as_bytes()); - let salt = hasher.finalize().to_vec(); - - get_instantiate2_address( - deps.as_ref(), - env.contract.address.as_str(), - &salt, - cw721_code_id, - ) - }?; - - generate_receive_callback_msg( - deps.as_ref(), - &cloned_data, - receive_callback_data, - receive_callback_addr, - nft_contract.to_string(), - ) - } else { - None - }; - - let incoming_proxy_msg = - get_incoming_proxy_msg(deps.storage, packet.clone(), cloned_data.clone())?; + let receiver = deps.api.addr_validate(&data.receiver)?; let voucher_and_channel_messages = match is_redemption { true => { let redemption = VoucherRedemption { @@ -206,26 +212,51 @@ pub(crate) fn receive_ibc_packet( } }; - let submessage = into_submessage( - env.contract.address, - voucher_and_channel_messages.0, - voucher_and_channel_messages.1, - callback_msg, - incoming_proxy_msg, - )?; + Ok((is_redemption, voucher_and_channel_messages)) +} - let response = if let Some(memo) = data.memo { - IbcReceiveResponse::default().add_attribute("ics721_memo", memo) - } else { - IbcReceiveResponse::default() - }; +fn create_callback_msg( + deps: Deps, + env: &Env, + data: &NonFungibleTokenPacketData, + is_redemption: bool, + callback: Option<(Binary, Option)>, + local_class_id: ClassId, +) -> Result, ContractError> { + if let Some((receive_callback_data, receive_callback_addr)) = callback { + // callback require the nft contract, get it using the class id from the action + let nft_contract = if is_redemption { + // If its a redemption, it means we already have the contract address in storage - Ok(response - .add_submessage(submessage) - .add_attribute("method", "receive_ibc_packet") - .add_attribute("class_id", data.class_id) - .add_attribute("local_channel", packet.dest.channel_id) - .add_attribute("counterparty_channel", packet.src.channel_id)) + CLASS_ID_TO_NFT_CONTRACT + .load(deps.storage, local_class_id.clone()) + .map_err(|_| ContractError::NoNftContractForClassId(local_class_id.to_string())) + } else { + // If its a creation action, we can use the instantiate2 function to get the nft contract + // we don't care of the contract is instantiated yet or not, as later submessage will instantiate it if its not. + // The reason we use instantiate2 here is because we don't know if it was already instantiated or not. + + let cw721_code_id = CW721_CODE_ID.load(deps.storage)?; + // for creating a predictable nft contract using, using instantiate2, we need: checksum, creator, and salt: + // - using class id as salt for instantiating nft contract guarantees a) predictable address and b) uniqueness + // for this salt must be of length 32 bytes, so we use sha256 to hash class id + let mut hasher = Sha256::new(); + hasher.update(local_class_id.as_bytes()); + let salt = hasher.finalize().to_vec(); + + get_instantiate2_address(deps, env.contract.address.as_str(), &salt, cw721_code_id) + }?; + + Ok(generate_receive_callback_msg( + deps, + data, + receive_callback_data, + receive_callback_addr, + nft_contract.to_string(), + )) + } else { + Ok(None) + } } pub fn into_submessage( From 31ab7b50dc44ee82cd40c5153cb6302b702d48f6 Mon Sep 17 00:00:00 2001 From: mr-t Date: Sun, 28 Jan 2024 00:35:54 +0100 Subject: [PATCH 08/14] docs --- packages/ics721/src/ibc_packet_receive.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/ics721/src/ibc_packet_receive.rs b/packages/ics721/src/ibc_packet_receive.rs index 07a95bac..366274dd 100644 --- a/packages/ics721/src/ibc_packet_receive.rs +++ b/packages/ics721/src/ibc_packet_receive.rs @@ -34,7 +34,6 @@ pub(crate) fn receive_ibc_packet( // Check if NFT is local if not get the local class id let maybe_local_class_id = try_pop_source_prefix(&packet.src, &data.class_id); let callback = get_receive_callback(&data); - // If class is not local, its something new let local_class_id = if let Some(local_class_id) = maybe_local_class_id { ClassId::new(local_class_id) } else { @@ -42,9 +41,8 @@ pub(crate) fn receive_ibc_packet( ClassId::new(format!("{}{}", local_prefix, data.class_id)) }; - let incoming_proxy_msg = - get_incoming_proxy_msg(deps.as_ref().storage, packet.clone(), data.clone())?; - + // sub message holds 2 to 4 messages: + // - one message for voucher creation or redemption, another message for updating incoming or outgoing channel let (is_redemption, voucher_and_channel_messages) = create_voucher_and_channel_messages( deps.as_ref(), env.clone(), @@ -53,8 +51,10 @@ pub(crate) fn receive_ibc_packet( local_class_id.clone(), packet.clone(), )?; - - // if there is a callback, generate the callback message + // - one optional incoming proxy message + let incoming_proxy_msg = + get_incoming_proxy_msg(deps.as_ref().storage, packet.clone(), data.clone())?; + // - one optional callback message let callback_msg = create_callback_msg( deps.as_ref(), &env, @@ -266,7 +266,7 @@ pub fn into_submessage( callback_msg: Option, incoming_proxy_msg: Option, ) -> StdResult> { - let mut operands = Vec::with_capacity(3); // 3 is the max number of submessages we can have + let mut operands = Vec::with_capacity(4); // 4 is the max number of submessages we can have if let Some(incoming_proxy_msg) = incoming_proxy_msg { operands.push(incoming_proxy_msg) } From dec10c18c4eff225edc9f166f194cfc9b7dbce3b Mon Sep 17 00:00:00 2001 From: mr-t Date: Sun, 28 Jan 2024 00:38:28 +0100 Subject: [PATCH 09/14] cleanup --- ics721_bug.md | 77 --------------------------------------------------- 1 file changed, 77 deletions(-) delete mode 100644 ics721_bug.md diff --git a/ics721_bug.md b/ics721_bug.md deleted file mode 100644 index 46619f72..00000000 --- a/ics721_bug.md +++ /dev/null @@ -1,77 +0,0 @@ -Bug in ICS721 when an NFT gets transferred back from chain B to A. - -Preliminary: NFT (forward) transferred from chain A to B -- NFT outcoume: NFT escrowed by ICS721 on chain A, NFT minted on newly, instantiated collection on chain B -- state changes: - - entry added on chain A: `OUTGOING_CLASS_TOKEN_TO_CHANNEL` (marker, check for identifying next time, NFT gets transferred back) - - entry added on chain B: `INCOMING_CLASS_TOKEN_TO_CHANNEL` - -Expected result on back transfer -- NFT burned on chain B -- "source/OG" NFT escrowed by ICS721 transferred to given recipent -- state changes: - - removed entry in `INCOMING_CLASS_TOKEN_TO_CHANNEL` on chain A - - removed entry in `OUTGOING_CLASS_TOKEN_TO_CHANNEL` on chain B -Actual result: -- NFT burned on chain B -- NFT minted on newly, instantiated collection -- NFT escrowed still escrowed by ICS721 on source collection - - - -By the spec of ics721, it is possible doing bulk transfers (whilst cw721 doesnt support this yet). -I believe this was the main objection of `ActionAggregator`'s design: -1. for each NFT either an `Action::Redemption` or `Action::Creation` is created: - - `Action::Redemption`: unescrow nft on forward transfer or - - `Action::Creation`: mint NFT and instantiate collection - - in addition for redemption/back transfer, entries in `OUTGOING_CLASS_TOKEN_TO_CHANNEL` are removed. That's the bug in contract's tx, we've identified -2. Each NFT action added to `ActionAggregator` - - interestingly `Action` enums are converted to either `VoucherRedemption` or `VoucherCreation` structs - - then converted structs are added to aggregator - - imo conversion not needed here, better approach here: -3. Finally all gets wrapped into a single sub message: - - create message list for each recreation or creation struct in aggregator: - - convert to WasmMsg with: - - `ExecuteMsg::Callback(CallbackMsg::CreateVouchers)` or - - `ExecuteMsg::Callback(CallbackMsg::RedeemVouchers)` - - message list represent a list of nft `operands` - - optional incoming proxy is added on top of operands list - - optional callback is added at the end of operands list - - merge message list into into single, final sub message - - final sub message is of type `reply all` for making sure TX always succeeds - - if there's only one message in list, this will be used for final sub message - - if message list contains more than one entries, they are merged into `ExecuteMsg::Callback(CallbackMsg::Conjunction {operands})` - -As a result a single `CallbackMsg` sub msg is created, which is either a -- `CallbackMsg::CreateVouchers` - - appends optional instantiate sub msg - - appends `CallbackMsg::Mint` msg -- `CallbackMsg::RedeemVouchers` - - transfer NFT to recipient -- `CallbackMsg::Conjunction` - - appends all to messages (operands callbacks (`CreateVouchers` or `RedeemVouchers` + optional incoming proxy + optional callback)) - -This guarantees: -- ics721 contract always succeeds -- each sub message handled serateley -- in case of sub msg failure - - it's partial state is reverted, but not its parent - - read more here: https://github.com/CosmWasm/cosmwasm/blob/main/SEMANTICS.md#submessages) - "... On an error, the subcall will revert any partial state changes due to this message, but not revert any state changes in the calling contract. The error may then be intercepted by the calling contract (for ReplyOn::Always and ReplyOn::Error). In this case, the messages error doesn't abort the whole transaction ..." - - remaining sub messages are not executed - - since operands in conjunction sub msg are added as messages (not sub msgs): - - sub msg is parent of operand messages - - ics721 is root parent of sub msg - - if any msg fails (eg. msg1: tranfer NFT1, msg2: tranfer NFT2) - - all messages and its parent/sub message is reverted - - root parent/ics721 is not reverted - -tl;dr: -- `receive_ibc_packet` response contains single callback sub msg, which: -- in case of failure, revert its own partial state, but wont revert contract's TX -- contains one or more messages - -The more I think about it, we can create this result response: -- create directly call back msgs -- hence, no intermediate step of Action enums and Action is required - From 94557a54b42dfe06fa021ab9457e4b8f356e0048 Mon Sep 17 00:00:00 2001 From: mr-t Date: Sun, 28 Jan 2024 01:18:00 +0100 Subject: [PATCH 10/14] cargo schema --- packages/ics721/schema/ics721.json | 37 +++++++++++++++++++++++++ packages/ics721/schema/raw/execute.json | 37 +++++++++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/packages/ics721/schema/ics721.json b/packages/ics721/schema/ics721.json index d5b0577a..7b5e8e18 100644 --- a/packages/ics721/schema/ics721.json +++ b/packages/ics721/schema/ics721.json @@ -246,6 +246,7 @@ "additionalProperties": false }, { + "description": "Redeem all entries in outgoing channel.", "type": "object", "required": [ "redeem_outgoing_channel_entries" @@ -270,6 +271,42 @@ }, "additionalProperties": false }, + { + "description": "Save all entries in incoming channel.", + "type": "object", + "required": [ + "add_incoming_channel_entries" + ], + "properties": { + "add_incoming_channel_entries": { + "type": "array", + "items": { + "type": "array", + "items": [ + { + "type": "array", + "items": [ + { + "$ref": "#/definitions/ClassId" + }, + { + "$ref": "#/definitions/TokenId" + } + ], + "maxItems": 2, + "minItems": 2 + }, + { + "type": "string" + } + ], + "maxItems": 2, + "minItems": 2 + } + } + }, + "additionalProperties": false + }, { "description": "Mints a NFT of collection class_id for receiver with the provided id and metadata. Only callable by this contract.", "type": "object", diff --git a/packages/ics721/schema/raw/execute.json b/packages/ics721/schema/raw/execute.json index 0f266cab..8347abe9 100644 --- a/packages/ics721/schema/raw/execute.json +++ b/packages/ics721/schema/raw/execute.json @@ -113,6 +113,7 @@ "additionalProperties": false }, { + "description": "Redeem all entries in outgoing channel.", "type": "object", "required": [ "redeem_outgoing_channel_entries" @@ -137,6 +138,42 @@ }, "additionalProperties": false }, + { + "description": "Save all entries in incoming channel.", + "type": "object", + "required": [ + "add_incoming_channel_entries" + ], + "properties": { + "add_incoming_channel_entries": { + "type": "array", + "items": { + "type": "array", + "items": [ + { + "type": "array", + "items": [ + { + "$ref": "#/definitions/ClassId" + }, + { + "$ref": "#/definitions/TokenId" + } + ], + "maxItems": 2, + "minItems": 2 + }, + { + "type": "string" + } + ], + "maxItems": 2, + "minItems": 2 + } + } + }, + "additionalProperties": false + }, { "description": "Mints a NFT of collection class_id for receiver with the provided id and metadata. Only callable by this contract.", "type": "object", From c5a6548ae2364d547189cf3334e34b45494b83da Mon Sep 17 00:00:00 2001 From: mr-t Date: Mon, 29 Jan 2024 12:22:07 +0100 Subject: [PATCH 11/14] 2 new admin msgs for fixing forked NFTs --- packages/ics721/src/error.rs | 15 ++ packages/ics721/src/execute.rs | 163 ++++++++++++++++++ packages/ics721/src/msg.rs | 19 ++ .../ics721/src/testing/integration_tests.rs | 161 ++++++++++++++++- 4 files changed, 355 insertions(+), 3 deletions(-) diff --git a/packages/ics721/src/error.rs b/packages/ics721/src/error.rs index 5f7d04e0..23604e12 100644 --- a/packages/ics721/src/error.rs +++ b/packages/ics721/src/error.rs @@ -27,6 +27,13 @@ pub enum ContractError { #[error("NFT not escrowed by ICS721! Owner: {0}")] NotEscrowedByIcs721(String), + #[error("{recipient} not owner of NFT {token_id}! Owner: {owner}")] + NotOwnerOfNft { + recipient: String, + owner: String, + token_id: String, + }, + #[error("only unordered channels are supported")] OrderedChannel {}, @@ -50,4 +57,12 @@ pub enum ContractError { #[error("Couldn't find nft contract for this class id: {0}")] NoNftContractForClassId(String), + + #[error("Unknown nft contract: {child_collection}, Class Id: {class_id}, Token ID: {token_id} => NFT contract: {cw721_addr}")] + NoClassIdForNftContract { + child_collection: String, + class_id: String, + token_id: String, + cw721_addr: String, + }, } diff --git a/packages/ics721/src/execute.rs b/packages/ics721/src/execute.rs index 7d6aeabd..f1ab276c 100644 --- a/packages/ics721/src/execute.rs +++ b/packages/ics721/src/execute.rs @@ -98,7 +98,170 @@ where }) => self.execute_receive_nft(deps, env, info, token_id, sender, msg), ExecuteMsg::Pause {} => self.execute_pause(deps, info), ExecuteMsg::Callback(msg) => self.execute_callback(deps, env, info, msg), + ExecuteMsg::AdminCleanAndBurnNft { + recipient, + token_id, + class_id, + collection, + } => self.execute_admin_clean_and_burn_nft( + deps, env, info, recipient, token_id, class_id, collection, + ), + ExecuteMsg::AdminCleanAndUnescrowNft { + recipient, + token_id, + class_id, + collection, + } => self.execute_admin_clean_and_unescrow_nft( + deps, env, info, recipient, token_id, class_id, collection, + ), + } + } + + #[allow(clippy::too_many_arguments)] + fn execute_admin_clean_and_burn_nft( + &self, + deps: DepsMut, + env: Env, + info: MessageInfo, + recipient: String, + token_id: String, + child_class_id: String, + child_collection: String, + ) -> Result, ContractError> { + deps.api.addr_validate(&recipient)?; + // only admin can call this method + let ContractInfoResponse { admin, .. } = deps + .querier + .query_wasm_contract_info(env.contract.address.to_string())?; + if admin.is_some() && info.sender != admin.unwrap() { + return Err(ContractError::Unauthorized {}); + } + + // check given child class id and child collection is the same as stored in the contract + let token_id = TokenId::new(token_id); + let child_class_id = ClassId::new(child_class_id); + let child_collection = deps.api.addr_validate(&child_collection)?; + let cw721_addr = CLASS_ID_TO_NFT_CONTRACT.load(deps.storage, child_class_id.clone())?; + if cw721_addr != child_collection { + return Err(ContractError::NoClassIdForNftContract { + child_collection: child_collection.to_string(), + class_id: child_class_id.to_string(), + token_id: token_id.into(), + cw721_addr: cw721_addr.to_string(), + }); } + + // remove incoming channel entry and metadata + INCOMING_CLASS_TOKEN_TO_CHANNEL + .remove(deps.storage, (child_class_id.clone(), token_id.clone())); + TOKEN_METADATA.remove(deps.storage, (child_class_id.clone(), token_id.clone())); + + // check NFT on child collection owned by recipient + let maybe_nft_info: Option = deps + .querier + .query_wasm_smart( + child_collection.clone(), + &cw721::Cw721QueryMsg::AllNftInfo { + token_id: token_id.clone().into(), + include_expired: None, + }, + ) + .ok(); + + let mut response = + Response::default().add_attribute("method", "execute_admin_clean_and_burn_nft"); + if let Some(UniversalAllNftInfoResponse { access, .. }) = maybe_nft_info { + if access.owner != recipient { + return Err(ContractError::NotOwnerOfNft { + recipient: recipient.to_string(), + token_id: token_id.clone().into(), + owner: access.owner.to_string(), + }); + } + // burn child NFT + let burn_msg = WasmMsg::Execute { + contract_addr: child_collection.to_string(), + msg: to_json_binary(&cw721::Cw721ExecuteMsg::Burn { + token_id: token_id.clone().into(), + })?, + funds: vec![], + }; + response = response.add_message(burn_msg); + } + + Ok(response) + } + + #[allow(clippy::too_many_arguments)] + fn execute_admin_clean_and_unescrow_nft( + &self, + deps: DepsMut, + env: Env, + info: MessageInfo, + recipient: String, + token_id: String, + home_class_id: String, + home_collection: String, + ) -> Result, ContractError> { + deps.api.addr_validate(&recipient)?; + // only admin can call this method + let ContractInfoResponse { admin, .. } = deps + .querier + .query_wasm_contract_info(env.contract.address.to_string())?; + if admin.is_some() && info.sender != admin.unwrap() { + return Err(ContractError::Unauthorized {}); + } + + // check given home class id and home collection is the same as stored in the contract + let home_class_id = ClassId::new(home_class_id); + let home_collection = deps.api.addr_validate(&home_collection)?; + let cw721_addr = CLASS_ID_TO_NFT_CONTRACT.load(deps.storage, home_class_id.clone())?; + if cw721_addr != home_collection { + return Err(ContractError::NoClassIdForNftContract { + child_collection: home_collection.to_string(), + class_id: home_class_id.to_string(), + token_id, + cw721_addr: cw721_addr.to_string(), + }); + } + + // remove outgoing channel entry + let token_id = TokenId::new(token_id); + OUTGOING_CLASS_TOKEN_TO_CHANNEL + .remove(deps.storage, (home_class_id.clone(), token_id.clone())); + + // check NFT on home collection owned by ics721 contract + let maybe_nft_info: Option = deps + .querier + .query_wasm_smart( + home_collection.clone(), + &cw721::Cw721QueryMsg::AllNftInfo { + token_id: token_id.clone().into(), + include_expired: None, + }, + ) + .ok(); + + let mut response = + Response::default().add_attribute("method", "execute_admin_clean_and_unescrow_nft"); + if let Some(UniversalAllNftInfoResponse { access, .. }) = maybe_nft_info { + if access.owner != env.contract.address { + return Err(ContractError::NotEscrowedByIcs721(access.owner.to_string())); + } + // transfer NFT + let transfer_msg = WasmMsg::Execute { + contract_addr: home_collection.to_string(), + msg: to_json_binary(&cw721::Cw721ExecuteMsg::TransferNft { + recipient: recipient.to_string(), + token_id: token_id.clone().into(), + })?, + funds: vec![], + }; + + response = response.add_message(transfer_msg); + } + + Ok(response) } /// ICS721 may receive an NFT from 2 sources: diff --git a/packages/ics721/src/msg.rs b/packages/ics721/src/msg.rs index b8832e0c..d531fccb 100644 --- a/packages/ics721/src/msg.rs +++ b/packages/ics721/src/msg.rs @@ -46,6 +46,25 @@ pub enum ExecuteMsg { /// Mesages used internally by the contract. These may only be /// called by the contract itself. Callback(CallbackMsg), + + /// Admin msg in case something goes wrong. + /// As a minimum it clean up states (incoming channel and token metadata), and burn NFT if exists. + AdminCleanAndBurnNft { + recipient: String, + token_id: String, + class_id: String, + collection: String, + }, + + /// Admin msg in case something goes wrong. + /// As a minimum it clean up state (outgoing channel), and transfer NFT if exists. + /// - transfer NFT if exists + AdminCleanAndUnescrowNft { + recipient: String, + token_id: String, + class_id: String, + collection: String, + }, } #[cw_serde] diff --git a/packages/ics721/src/testing/integration_tests.rs b/packages/ics721/src/testing/integration_tests.rs index 7b292c15..2167b4d6 100644 --- a/packages/ics721/src/testing/integration_tests.rs +++ b/packages/ics721/src/testing/integration_tests.rs @@ -21,7 +21,7 @@ use crate::{ ibc::Ics721Ibc, msg::{CallbackMsg, ExecuteMsg, InstantiateMsg, MigrateMsg, QueryMsg}, query::Ics721Query, - state::CollectionData, + state::{CollectionData, UniversalAllNftInfoResponse}, token_types::VoucherCreation, ContractError, }; @@ -353,11 +353,11 @@ impl Test { incoming_proxy, outgoing_proxy, pauser: admin.clone(), - cw721_admin: admin, + cw721_admin: admin.clone(), }, &[], "ics721-base", - admin_and_pauser.map(|p| app.api().addr_make(&p).to_string()), + admin.clone(), ) .unwrap(); @@ -510,6 +510,19 @@ impl Test { .unwrap() } + fn query_cw721_all_nft_info(&mut self, token_id: String) -> UniversalAllNftInfoResponse { + self.app + .wrap() + .query_wasm_smart( + self.source_cw721.clone(), + &cw721_base::msg::QueryMsg::::AllNftInfo { + token_id, + include_expired: None, + }, + ) + .unwrap() + } + fn execute_cw721_mint(&mut self, owner: Addr) -> Result { self.nfts_minted += 1; @@ -1992,6 +2005,148 @@ fn test_receive_nft() { } } +#[test] +fn test_admin_clean_and_unescrow_nft() { + // test case: receive nft from cw721-base + { + let mut test = Test::new( + false, + false, + None, + Some(ICS721_ADMIN_AND_PAUSER.to_string()), + cw721_base_contract(), + true, + ); + // simplify: mint and escrowed/owned by ics721, as a precondition for receive nft + let token_id_escrowed_by_ics721 = test.execute_cw721_mint(test.ics721.clone()).unwrap(); + let recipient = test.app.api().addr_make("recipient"); + let token_id_from_owner = test.execute_cw721_mint(recipient.clone()).unwrap(); + let channel = "channel-0".to_string(); + test.app + .execute_contract( + test.source_cw721.clone(), + test.ics721.clone(), + &ExecuteMsg::ReceiveNft(cw721::Cw721ReceiveMsg { + sender: test.source_cw721_owner.to_string(), + token_id: token_id_escrowed_by_ics721.clone(), + msg: to_json_binary(&IbcOutgoingMsg { + receiver: NFT_OWNER_TARGET_CHAIN.to_string(), // nft owner for other chain, on this chain ics721 is owner + channel_id: channel.clone(), + timeout: IbcTimeout::with_block(IbcTimeoutBlock { + revision: 0, + height: 10, + }), + memo: None, + }) + .unwrap(), + }), + &[], + ) + .unwrap(); + // check outgoing channel entry + let outgoing_channel = test.query_outgoing_channels(); + assert_eq!(outgoing_channel.len(), 1); + let class_id = ClassId::new(test.source_cw721.to_string()); + assert_eq!( + outgoing_channel, + vec![( + (class_id.to_string(), token_id_escrowed_by_ics721.clone()), + channel.clone() + )] + ); + // assert nft is escrowed + let UniversalAllNftInfoResponse { access, .. } = + test.query_cw721_all_nft_info(token_id_escrowed_by_ics721.clone()); + assert_eq!(access.owner, test.ics721.to_string()); + + // non admin can't call + let non_admin = test.app.api().addr_make("not_admin"); + let admin = test.app.api().addr_make(ICS721_ADMIN_AND_PAUSER); + let clean_and_burn_msg = ExecuteMsg::AdminCleanAndBurnNft { + recipient: recipient.to_string(), + token_id: token_id_escrowed_by_ics721.clone(), + class_id: class_id.to_string(), + collection: test.source_cw721.to_string(), + }; + let err: ContractError = test + .app + .execute_contract( + non_admin.clone(), + test.ics721.clone(), + &clean_and_burn_msg, + &[], + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!(err, ContractError::Unauthorized {}); + + let clean_and_unescrow_msg = ExecuteMsg::AdminCleanAndUnescrowNft { + recipient: recipient.to_string(), + token_id: token_id_from_owner.clone(), // not escrowed by ics721 + class_id: class_id.to_string(), + collection: test.source_cw721.to_string(), + }; + let err: ContractError = test + .app + .execute_contract( + admin.clone(), + test.ics721.clone(), + &clean_and_unescrow_msg, + &[], + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!( + err, + ContractError::NotEscrowedByIcs721(recipient.to_string()) + ); + + // unknown class id + let clean_and_unescrow_msg = ExecuteMsg::AdminCleanAndUnescrowNft { + recipient: recipient.to_string(), + token_id: token_id_escrowed_by_ics721.to_string(), + class_id: "unknown".to_string(), + collection: test.source_cw721.to_string(), + }; + let err: ContractError = test + .app + .execute_contract( + admin.clone(), + test.ics721.clone(), + &clean_and_unescrow_msg, + &[], + ) + .unwrap_err() + .downcast() + .unwrap(); + assert_eq!(err, ContractError::Std(StdError::NotFound { kind: "type: cosmwasm_std::addresses::Addr; key: [00, 01, 65, 75, 6E, 6B, 6E, 6F, 77, 6E]".to_string() })); + + let clean_and_unescrow_msg = ExecuteMsg::AdminCleanAndUnescrowNft { + recipient: recipient.to_string(), + token_id: token_id_escrowed_by_ics721.clone(), + class_id: class_id.to_string(), + collection: test.source_cw721.to_string(), + }; + test.app + .execute_contract( + admin.clone(), + test.ics721.clone(), + &clean_and_unescrow_msg, + &[], + ) + .unwrap(); + // asert outgoing channel entry is removed + let outgoing_channel = test.query_outgoing_channels(); + assert_eq!(outgoing_channel.len(), 0); + // check nft is unescrowed + let UniversalAllNftInfoResponse { access, .. } = + test.query_cw721_all_nft_info(token_id_escrowed_by_ics721.clone()); + assert_eq!(access.owner, recipient.to_string()); + } +} + /// In case proxy for ICS721 is defined, ICS721 only accepts receival from proxy - not from nft contract! #[test] fn test_no_receive_with_proxy() { From 80995edf52d7d34931394ba54fe28055dd4a2468 Mon Sep 17 00:00:00 2001 From: mr-t Date: Mon, 29 Jan 2024 12:28:21 +0100 Subject: [PATCH 12/14] docs --- packages/ics721/src/execute.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/ics721/src/execute.rs b/packages/ics721/src/execute.rs index f1ab276c..9112b73b 100644 --- a/packages/ics721/src/execute.rs +++ b/packages/ics721/src/execute.rs @@ -179,6 +179,7 @@ where }); } // burn child NFT + // note: this requires approval from recipient, or recipient burns it himself let burn_msg = WasmMsg::Execute { contract_addr: child_collection.to_string(), msg: to_json_binary(&cw721::Cw721ExecuteMsg::Burn { From dc32c5f59b8cf69edd64cbc65043f9826d76d84a Mon Sep 17 00:00:00 2001 From: mr-t Date: Mon, 29 Jan 2024 15:35:39 +0100 Subject: [PATCH 13/14] test admin msgs --- packages/ics721/src/execute.rs | 12 +- packages/ics721/src/msg.rs | 2 +- .../ics721/src/testing/integration_tests.rs | 2 +- ts-relayer-tests/src/cw721-utils.ts | 33 +++ ts-relayer-tests/src/ics721-utils.ts | 50 +++++ ts-relayer-tests/src/ics721.spec.ts | 189 +++++++++++++++++- 6 files changed, 279 insertions(+), 9 deletions(-) diff --git a/packages/ics721/src/execute.rs b/packages/ics721/src/execute.rs index 9112b73b..31a789a5 100644 --- a/packages/ics721/src/execute.rs +++ b/packages/ics721/src/execute.rs @@ -99,12 +99,12 @@ where ExecuteMsg::Pause {} => self.execute_pause(deps, info), ExecuteMsg::Callback(msg) => self.execute_callback(deps, env, info, msg), ExecuteMsg::AdminCleanAndBurnNft { - recipient, + owner, token_id, class_id, collection, } => self.execute_admin_clean_and_burn_nft( - deps, env, info, recipient, token_id, class_id, collection, + deps, env, info, owner, token_id, class_id, collection, ), ExecuteMsg::AdminCleanAndUnescrowNft { recipient, @@ -123,12 +123,12 @@ where deps: DepsMut, env: Env, info: MessageInfo, - recipient: String, + owner: String, token_id: String, child_class_id: String, child_collection: String, ) -> Result, ContractError> { - deps.api.addr_validate(&recipient)?; + deps.api.addr_validate(&owner)?; // only admin can call this method let ContractInfoResponse { admin, .. } = deps .querier @@ -171,9 +171,9 @@ where let mut response = Response::default().add_attribute("method", "execute_admin_clean_and_burn_nft"); if let Some(UniversalAllNftInfoResponse { access, .. }) = maybe_nft_info { - if access.owner != recipient { + if access.owner != owner { return Err(ContractError::NotOwnerOfNft { - recipient: recipient.to_string(), + recipient: owner.to_string(), token_id: token_id.clone().into(), owner: access.owner.to_string(), }); diff --git a/packages/ics721/src/msg.rs b/packages/ics721/src/msg.rs index d531fccb..ccc9ab75 100644 --- a/packages/ics721/src/msg.rs +++ b/packages/ics721/src/msg.rs @@ -50,7 +50,7 @@ pub enum ExecuteMsg { /// Admin msg in case something goes wrong. /// As a minimum it clean up states (incoming channel and token metadata), and burn NFT if exists. AdminCleanAndBurnNft { - recipient: String, + owner: String, token_id: String, class_id: String, collection: String, diff --git a/packages/ics721/src/testing/integration_tests.rs b/packages/ics721/src/testing/integration_tests.rs index 2167b4d6..67c5f4e8 100644 --- a/packages/ics721/src/testing/integration_tests.rs +++ b/packages/ics721/src/testing/integration_tests.rs @@ -2063,7 +2063,7 @@ fn test_admin_clean_and_unescrow_nft() { let non_admin = test.app.api().addr_make("not_admin"); let admin = test.app.api().addr_make(ICS721_ADMIN_AND_PAUSER); let clean_and_burn_msg = ExecuteMsg::AdminCleanAndBurnNft { - recipient: recipient.to_string(), + owner: recipient.to_string(), token_id: token_id_escrowed_by_ics721.clone(), class_id: class_id.to_string(), collection: test.source_cw721.to_string(), diff --git a/ts-relayer-tests/src/cw721-utils.ts b/ts-relayer-tests/src/cw721-utils.ts index 7a78d972..ce447f00 100644 --- a/ts-relayer-tests/src/cw721-utils.ts +++ b/ts-relayer-tests/src/cw721-utils.ts @@ -48,6 +48,29 @@ export function sendNft( ); } +export function approve( + client: CosmWasmSigner, + cw721Contract: string, + spender: string, + token_id: string +) { + // msg to be executed on cw721 contract + const msg = { + approve: { + token_id, + spender, + }, + }; + return client.sign.execute( + client.senderAddress, + cw721Contract, + msg, + "auto", // fee + undefined, // no memo + undefined // no funds + ); +} + // ######### query export function allTokens( client: CosmWasmSigner, @@ -104,3 +127,13 @@ export function ownerOf( }; return client.sign.queryContractSmart(cw721Contract, msg); } + +export function numTokens( + client: CosmWasmSigner, + cw721Contract: string +): Promise<{ count: number }> { + const msg = { + num_tokens: {}, + }; + return client.sign.queryContractSmart(cw721Contract, msg); +} diff --git a/ts-relayer-tests/src/ics721-utils.ts b/ts-relayer-tests/src/ics721-utils.ts index 9192c40b..e8142554 100644 --- a/ts-relayer-tests/src/ics721-utils.ts +++ b/ts-relayer-tests/src/ics721-utils.ts @@ -42,6 +42,56 @@ export function migrateIncomingProxy( ); } +export function adminCleanAndUnescrowNft( + client: CosmWasmSigner, + contractAddress: string, + recipient: string, + token_id: string, + class_id: string, + collection: string +) { + const msg = { + admin_clean_and_unescrow_nft: { + recipient, + token_id, + class_id, + collection, + }, + }; + return client.sign.execute( + client.senderAddress, + contractAddress, + msg, + "auto", + undefined + ); +} + +export function adminCleanAndBurnNft( + client: CosmWasmSigner, + contractAddress: string, + owner: string, + token_id: string, + class_id: string, + collection: string +) { + const msg = { + admin_clean_and_burn_nft: { + owner, + token_id, + class_id, + collection, + }, + }; + return client.sign.execute( + client.senderAddress, + contractAddress, + msg, + "auto", + undefined + ); +} + // ######### query export function nftContracts( client: CosmWasmSigner, diff --git a/ts-relayer-tests/src/ics721.spec.ts b/ts-relayer-tests/src/ics721.spec.ts index 0068fe01..746ff153 100644 --- a/ts-relayer-tests/src/ics721.spec.ts +++ b/ts-relayer-tests/src/ics721.spec.ts @@ -4,8 +4,10 @@ import anyTest, { ExecutionContext, TestFn } from "ava"; import { Order } from "cosmjs-types/ibc/core/channel/v1/channel"; import { instantiateContract } from "./controller"; -import { allTokens, mint, ownerOf, sendNft } from "./cw721-utils"; +import { allTokens, approve, mint, ownerOf, sendNft } from "./cw721-utils"; import { + adminCleanAndBurnNft, + adminCleanAndUnescrowNft, incomingChannels, migrate, migrateIncomingProxy, @@ -744,6 +746,191 @@ test.serial("transfer NFT: wasmd -> osmo", async (t) => { t.is(wasmAddr, tokenOwner.owner); }); +test.serial("admin unescrow and burn NFT: wasmd -> osmo", async (t) => { + await standardSetup(t); + + const { + wasmClient, + wasmAddr, + wasmCw721, + wasmIcs721, + wasmCw721OutgoingProxy, + osmoClient, + osmoAddr, + osmoIcs721, + channel, + } = t.context; + + const tokenId = "1"; + await mint(wasmClient, wasmCw721, tokenId, wasmAddr, undefined); + // assert token is minted + let tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); + t.is(wasmAddr, tokenOwner.owner); + + // ==== happy path: transfer NFT to osmo chain ==== + // test transfer NFT to osmo chain + t.log(`transfering to osmo chain via ${channel.channel.src.channelId}`); + const ibcMsg = { + receiver: osmoAddr, + channel_id: channel.channel.src.channelId, + timeout: { + block: { + revision: 1, + height: 90000, + }, + }, + }; + const transferResponse = await sendNft( + wasmClient, + wasmCw721, + wasmCw721OutgoingProxy, + ibcMsg, + tokenId + ); + t.truthy(transferResponse); + + // Relay and verify we got a success + t.log("relaying packets"); + const info = await channel.link.relayAll(); + assertAckSuccess(info.acksFromA); + + // assert NFT on chain A is locked/owned by ICS contract + tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); + t.is(wasmIcs721, tokenOwner.owner); + // assert NFT minted on chain B + const osmoClassId = `${channel.channel.dest.portId}/${channel.channel.dest.channelId}/${t.context.wasmCw721}`; + const osmoCw721 = await osmoClient.sign.queryContractSmart(osmoIcs721, { + nft_contract: { class_id: osmoClassId }, + }); + let allNFTs = await allTokens(osmoClient, osmoCw721); + t.is(allNFTs.tokens.length, 1, `all tokens: ${JSON.stringify(allNFTs)}`); + // assert NFT on chain B is owned by osmoAddr + tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId); + t.is(osmoAddr, tokenOwner.owner); + + const beforeWasmOutgoingClassTokenToChannelList = await outgoingChannels( + wasmClient, + wasmIcs721 + ); + // there should be one outgoing channel entry + t.deepEqual( + beforeWasmOutgoingClassTokenToChannelList, + [[[wasmCw721, tokenId], channel.channel.src.channelId]], + `wasm outgoing channels before: +- before: ${JSON.stringify(beforeWasmOutgoingClassTokenToChannelList)}` + ); + // no incoming channel entry + const beforeWasmIncomingClassTokenToChannelList = await incomingChannels( + wasmClient, + wasmIcs721 + ); + t.deepEqual( + beforeWasmIncomingClassTokenToChannelList, + [], + `wasm incoming channels before: +- before: ${JSON.stringify(beforeWasmIncomingClassTokenToChannelList)}` + ); + // one nft contract entry + const beforeWasmNftContractsToClassIdList = await nftContracts( + wasmClient, + wasmIcs721 + ); + t.deepEqual( + beforeWasmNftContractsToClassIdList, + [[wasmCw721, wasmCw721]], + `wasm nft contracts before: +- before: ${JSON.stringify(beforeWasmNftContractsToClassIdList)}` + ); + + // no outgoing channel entry + const beforeOsmoOutgoingClassTokenToChannelList = await outgoingChannels( + osmoClient, + osmoIcs721 + ); + t.deepEqual( + beforeOsmoOutgoingClassTokenToChannelList, + [], + `osmo outgoing channels before: +- before: ${JSON.stringify(beforeOsmoOutgoingClassTokenToChannelList)}` + ); + // there should be one incoming channel entry + const beforeOsmoIncomingClassTokenToChannelList = await incomingChannels( + osmoClient, + osmoIcs721 + ); + t.deepEqual( + beforeOsmoIncomingClassTokenToChannelList, + [[[osmoClassId, tokenId], channel.channel.dest.channelId]], + `osmo incoming channels before: +- before: ${JSON.stringify(beforeOsmoIncomingClassTokenToChannelList)}` + ); + // one nft contract entry + const beforeOsmoNftContractsToClassIdList = await nftContracts( + osmoClient, + osmoIcs721 + ); + t.deepEqual( + beforeOsmoNftContractsToClassIdList, + [[osmoClassId, osmoCw721]], + `osmo incoming channels before: +- before: ${JSON.stringify(beforeOsmoNftContractsToClassIdList)}` + ); + + // ==== test unescrow NFT on wasm chain ==== + t.log(`unescrow NFT on wasm chain`); + await adminCleanAndUnescrowNft( + wasmClient, + wasmIcs721, + wasmAddr, + tokenId, + wasmCw721, + wasmCw721 + ); + // there should be no outgoing channel entry + const afterWasmOutgoingClassTokenToChannelList = await outgoingChannels( + wasmClient, + wasmIcs721 + ); + t.deepEqual( + afterWasmOutgoingClassTokenToChannelList, + [], + `wasm outgoing channels after: +- after: ${JSON.stringify(afterWasmOutgoingClassTokenToChannelList)}` + ); + // assert NFT on chain A is owned by wasmAddr + tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId); + t.is(wasmAddr, tokenOwner.owner); + + // ==== test burn NFT on osmo chain ==== + // we need to approve the contract to burn the NFT + t.log(`approve NFT on osmo chain`); + const response = await approve(osmoClient, osmoCw721, osmoIcs721, tokenId); + t.log(`- response: ${JSON.stringify(response, bigIntReplacer, 2)}`); + t.log(`burn NFT on osmo chain`); + await adminCleanAndBurnNft( + osmoClient, + osmoIcs721, + osmoAddr, + tokenId, + osmoClassId, + osmoCw721 + ); + t.log(`- response: ${JSON.stringify(response, bigIntReplacer, 2)}`); + allNFTs = await allTokens(osmoClient, osmoCw721); + t.is(allNFTs.tokens.length, 0); + // there should be no incoming channel entry + const afterOsmoIncomingClassTokenToChannelList = await incomingChannels( + osmoClient, + osmoIcs721 + ); + t.deepEqual( + afterOsmoIncomingClassTokenToChannelList, + [], + `osmo incoming channels after: +- after: ${JSON.stringify(afterOsmoIncomingClassTokenToChannelList)}` + ); +}); + test.serial("malicious NFT", async (t) => { await standardSetup(t); const { From 545909fe950e6dbb93f62e0258a57adc7b90bddb Mon Sep 17 00:00:00 2001 From: mr-t Date: Mon, 29 Jan 2024 15:38:16 +0100 Subject: [PATCH 14/14] cargo schema --- packages/ics721/schema/ics721.json | 68 +++++++++++++++++++++++++ packages/ics721/schema/raw/execute.json | 68 +++++++++++++++++++++++++ 2 files changed, 136 insertions(+) diff --git a/packages/ics721/schema/ics721.json b/packages/ics721/schema/ics721.json index 7b5e8e18..56202f8d 100644 --- a/packages/ics721/schema/ics721.json +++ b/packages/ics721/schema/ics721.json @@ -174,6 +174,74 @@ } }, "additionalProperties": false + }, + { + "description": "Admin msg in case something goes wrong. As a minimum it clean up states (incoming channel and token metadata), and burn NFT if exists.", + "type": "object", + "required": [ + "admin_clean_and_burn_nft" + ], + "properties": { + "admin_clean_and_burn_nft": { + "type": "object", + "required": [ + "class_id", + "collection", + "owner", + "token_id" + ], + "properties": { + "class_id": { + "type": "string" + }, + "collection": { + "type": "string" + }, + "owner": { + "type": "string" + }, + "token_id": { + "type": "string" + } + }, + "additionalProperties": false + } + }, + "additionalProperties": false + }, + { + "description": "Admin msg in case something goes wrong. As a minimum it clean up state (outgoing channel), and transfer NFT if exists. - transfer NFT if exists", + "type": "object", + "required": [ + "admin_clean_and_unescrow_nft" + ], + "properties": { + "admin_clean_and_unescrow_nft": { + "type": "object", + "required": [ + "class_id", + "collection", + "recipient", + "token_id" + ], + "properties": { + "class_id": { + "type": "string" + }, + "collection": { + "type": "string" + }, + "recipient": { + "type": "string" + }, + "token_id": { + "type": "string" + } + }, + "additionalProperties": false + } + }, + "additionalProperties": false } ], "definitions": { diff --git a/packages/ics721/schema/raw/execute.json b/packages/ics721/schema/raw/execute.json index 8347abe9..652ac4a3 100644 --- a/packages/ics721/schema/raw/execute.json +++ b/packages/ics721/schema/raw/execute.json @@ -41,6 +41,74 @@ } }, "additionalProperties": false + }, + { + "description": "Admin msg in case something goes wrong. As a minimum it clean up states (incoming channel and token metadata), and burn NFT if exists.", + "type": "object", + "required": [ + "admin_clean_and_burn_nft" + ], + "properties": { + "admin_clean_and_burn_nft": { + "type": "object", + "required": [ + "class_id", + "collection", + "owner", + "token_id" + ], + "properties": { + "class_id": { + "type": "string" + }, + "collection": { + "type": "string" + }, + "owner": { + "type": "string" + }, + "token_id": { + "type": "string" + } + }, + "additionalProperties": false + } + }, + "additionalProperties": false + }, + { + "description": "Admin msg in case something goes wrong. As a minimum it clean up state (outgoing channel), and transfer NFT if exists. - transfer NFT if exists", + "type": "object", + "required": [ + "admin_clean_and_unescrow_nft" + ], + "properties": { + "admin_clean_and_unescrow_nft": { + "type": "object", + "required": [ + "class_id", + "collection", + "recipient", + "token_id" + ], + "properties": { + "class_id": { + "type": "string" + }, + "collection": { + "type": "string" + }, + "recipient": { + "type": "string" + }, + "token_id": { + "type": "string" + } + }, + "additionalProperties": false + } + }, + "additionalProperties": false } ], "definitions": {