public-awesome / cw-ics721 Goto Github PK

ics721 is pretty involved, diagrams to explain key flows and features will go a long way:

• Basic flow sending and receiving an NFT
• cw-ics721-bridge SubMessages flow

This seems to be critical where info.sender is set provided by proxy: https://github.com/public-awesome/ics721/blob/main/contracts/cw-ics721-bridge/src/contract.rs#L110

fn execute_receive_proxy_nft( ... ) -> Result<Response, ContractError> {
    if PROXY
        .load(deps.storage)?
        .map_or(true, |proxy| info.sender != proxy)
    {
        return Err(ContractError::Unauthorized {});
    }
    let mut info = info;
    info.sender = deps.api.addr_validate(&eyeball)?;
    let cw721::Cw721ReceiveMsg {
        token_id,
        sender,
        msg,
    } = msg;
    receive_nft(deps, info, TokenId::new(token_id), sender, msg)
}

So it would be possible having an exploited proxy contract and providing any random collection which will then be transferred to another chain.

One solution is checking, whether sender is owner of NFT by querying all_nft_info, instead of nft_info here:

https://github.com/public-awesome/ics721/blob/main/contracts/cw-ics721-bridge/src/contract.rs#L173

Tokens originating from a remote chain will have a class_id without a port or channel.

i think the CosmWasm documentation and code are a little self-contradictory about message ordering. this doesn't impact the functionality of this contract, but we'll need to update a comment in the code depending on the outcome of this:

CosmWasm/cosmwasm#1514

the comment:

https://github.com/public-awesome/ics721/blob/52852e07446c91983a91495e85103370f49814a7/contracts/cw-ics721-bridge/src/contract.rs#L356-L361

Is metadata transfer supported? I tried transferring token IDs (1, 2, 3) from stars1vujhvlfy7kzjewwuedhn0gsk5nem6y6rzk5xfr275k7nvyv54jlqm27e9s to Sei but their metadata got lost when they reached Sei. Here are the addresses created by Sei bridge:
1 - sei1vx654yrv3fzm0p5hmzmacnwx3th2gtaxzkvxc8susnmyvuc8v33s4jtl64
2 - sei1c27u39gakrjr96g2yvr2w9d9rtds2gan2cl4gcex9m2fakcy04rqse0tzv
3 - sei10u2ztpjuus9sv5vyu4hj0gaq5xvuxepuv0wtc5dv4yxjh4sn24nqlyp42n

Context: https://github.com/public-awesome/ics721/pull/22/files#diff-aa1c6c24578bd784194dbe1472d496f28bc4f5d992be6fbe43c921d3827cdf7bR111

There are good reasons being able to extend this contract. Like this one never returns an error: https://github.com/public-awesome/ics721/blob/zeke/ics721/contracts/cw-ics721-bridge/src/ibc.rs#L111-L115

pub fn ibc_packet_receive(
    deps: DepsMut,
    env: Env,
    msg: IbcPacketReceiveMsg,
) -> Result<IbcReceiveResponse, Never> {

Unfortunately only ContractError, but not Never, is exposed: https://github.com/public-awesome/ics721/blob/zeke/ics721/contracts/cw-ics721-bridge/src/lib.rs#L14

Useful links:

https://github.com/cosmos/ibc/pull/615/files

https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-043-nft-module.md

Context: #22 (comment)

@jhernandezb wrote:

should we have a governance sudo method that can restore closed channels ? This will happen more than you think currently for ics20 a proposal is submitted on chain to replace a closed channel for a valid one.

Reason: Channel trust period very low and failing to update light client states during this period can lead to clients to be expired.

Edit: nvm I think this is at the light client level but should be tested

We may want some governance sudo methods for such things as restoring closed channels or performing certain admin functions for managing this contract. Though perhaps, current Cosmos SDK governance around IBC is good enough.

This issue is to make sure we have a discussion around this, and at the very least test edge cases like closed channels.

https://github.com/public-awesome/ics721/blob/ff2be4e7ef8ec9a80023b7ac624662e792c4b02b/contracts/cw-ics721-bridge/src/contract.rs#L436

If none is provided then value for PROXY is null.

Problem

Spec seems saying camel case, but all cosmos ibc specs are saying like this but using snake case. Please refer below example.

interface NonFungibleTokenPacketData {
  classId: string
  classUri: string
  classData: string
  tokenIds: string[]
  tokenUris: string[]
  tokenData: string[]
  sender: string
  receiver: string
  memo: string
}

Cosmos implementation is using snake case

message NonFungibleTokenPacketData {
  // the class_id of class to be transferred
  string class_id = 1;
  // the class_uri of class to be transferred
  string class_uri = 2;
  // the class_data of class to be transferred
  string class_data = 3;
  // the non fungible tokens to be transferred
  repeated string token_ids = 4;
  // the non fungible tokens's uri to be transferred
  repeated string token_uris = 5;
  // the non fungible tokens's data to be transferred
  repeated string token_data = 6;
  // the sender address
  string sender = 7;
  // the recipient address on the destination chain
  string receiver = 8;
  // optional memo
  string memo = 9;
}

cw-ics721 is using camel case

Example

ibc-fee spec is saying

interface Acknowledgement {
    appAcknowledgement: []byte
    forwardRelayerAddress: string
    underlyingAppSuccess: boolean
}

but, actual implementation is using snake case

message IncentivizedAcknowledgement {
  // the underlying app acknowledgement bytes
  bytes app_acknowledgement = 1;
  // the relayer address which submits the recv packet message
  string forward_relayer_address = 2;
  // success flag of the base application callback
  bool underlying_app_success = 3;
}

Change:

• INCOMING_PROXY: Item<Option<Addr>> -> INCOMING_PROXY: Item<Addr>
• OUTGOING_PROXY: Item<Option<Addr>> -> OUTGOING_PROXY: Item<Addr>

Requires also migration!

Currently: during instantiation None is stored, and on reply addr is stored. By changing this, we can store addr during instantiation right away.

0.17 makes some improvements to the minter, metadata, and response attributes.

testing seems to reveal that it is possible for the other side of a channel to close in such a way that we can not stop:

https://github.com/public-awesome/ics721/blob/c0dd0a7f668af159ae0736d8ef5e67537d0983eb/e2e/suite_test.go#L715-L717

we should likely fail gracefully in this case and give an admin the ability to unlock distressed NFTs in that channel. we can give the admin the ability to do this on a per-channel basis so that this may only be done for channels we know to be closed.

Instead of class id being stored in name and symbol, it's actual name and symbol from source chain should be transferred, when creating new collection.

Unfortunately name can't easily be changed. Reason is here: https://github.com/public-awesome/cw-ics721/blob/main/packages/ics721/src/ibc.rs#L256-L262

Basically ICS721 does a sub msg for instantiating cw721 contract (aka collection) on target chain. Once done, on reply, it needs to store ClassId and collection address in a map. The only way of getting class id is storing it somewhere in cw721 - in this case in name.

So what I can at least do, is setting symbol from source source.
A possible solutions is storing name and class id into name with this syntax: name: name_from_source_collection (classID).

This way I could extract ClassId from name.

TBD: instantiate2

Makefile (or cargo-make) and scripts for:

• Deploying contracts to testnets
• Deploying proxy
• Configuration + setup
• Schema generation
• Typescript types generation with ts-codgen

I was trying transferring a Juno NFT to Stargaze and got this error:
submessages: Error parsing into type cw721::query::ContractInfoResponse: unknown field minter, expected nameorsymbol: execute wasm contract failed [CosmWasm/[email protected]/x/wasm/keeper/keeper.go:395]

Reason is ics721 uses standard cw721, expecting ContractInfoResponse which should only contain name and symbol. But Juno collection has more than that.

command: junod query wasm contract-state smart juno1kpsy0mh58fzrl973ndvppujs9ea5xe9wggjhhg07c42l0yqk5n7st5800h '{"contract_info": {}}'

output:

{
  "data": {
    "name": "Atlas",
    "symbol": "ATL",
    "minter": "juno1xw99es9dwshk4hak3g9hajkppqu4lwnahzs0lkyd6zq7amj23pjqd7pnte",
    "royalty_bps": [
      250,
      250
    ],
    "royalty_addrs": [
      "juno1u8sql7kmx5pget7tavwlz6hhe3mxqn0h8yqr2xx288mk5g4xy58qd32as2",
      "juno1hcldlknu2mn3exckkg75tyzjnderl95zyjte2wl495z9jla0rmdqegxlxx"
    ],
    "second_admin": "juno1u8sql7kmx5pget7tavwlz6hhe3mxqn0h8yqr2xx288mk5g4xy58qd32as2"
  }
}

Solution: instead of using restrictive cw_serde, we can use serde for being more lenient and ignore all unknown fields.

In the ibc3 feature, the new method now accepts 2 arguments, with the relayer addr as the 2nd.

Not sure what it means yet, but I think its need to be looked into.

Normally ICS721 must be instantiated with cw721-base code id: https://github.com/public-awesome/ics721/blob/main/contracts/cw-ics721-bridge/src/msg.rs#L7-L15

pub struct InstantiateMsg {
    /// Code ID of cw721-ics contract. A new cw721-ics will be
    /// instantiated for each new IBCd NFT classID.
    ///
    /// NOTE: this _must_ correspond to the cw721-base contract. Using
    /// a regular cw721 may cause the ICS 721 interface implemented by
    /// this contract to stop working, and IBCd away NFTs to be
    /// unreturnable as cw721 does not have a mint method in the spec.
    pub cw721_base_code_id: u64,

In case a non-cw721 contract is passed - which has no mint message - transfer should cause ACK fail and rollback transfer, so that NFT is returned to sender on other chain.

We have logic for checking on receive whether NonFungibleTokenPacketData is a back transfer. If we move this to a helper, we could extend proxies for checking and always allow backtransfer and skip WL checks.

I'm working on ICS-721 implementation in ibc-rs. I face two issues with decoding NonFungiblePacketData.

First, NonFungiblePacketData has Optional for some members. It causes null value in the packet data. Is it expected?

"{\"classId\":\"wasm14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9s0phg4d\",\"classUri\":null,\"classData\":\"eyJvd25lciI6Indhc20xNHpwMzVhdzl3Y256dGxoazUycGpjbHI2enJ2bXJja3pldXJhNWMiLCJjb250cmFjdF9pbmZvIjp7ImNvZGVfaWQiOjEsImNyZWF0b3IiOiJ3YXNtMTR6cDM1YXc5d2NuenRsaGs1MnBqY2xyNnpydm1yY2t6ZXVyYTVjIiwiYWRtaW4iOm51bGwsInBpbm5lZCI6ZmFsc2UsImliY19wb3J0IjpudWxsfSwibmFtZSI6InRlc3RfbmZ0Iiwic3ltYm9sIjoiVEVTVF9ORlQiLCJudW1fdG9rZW5zIjoxfQ==\",\"tokenIds\":[\"test_nft_0\"],\"tokenUris\":[\"http://example.com\"],\"tokenData\":null,\"sender\":\"wasm14zp35aw9wcnztlhk52pjclr6zrvmrckzeura5c\",\"receiver\":\"tnam1qq485cszjczr3379p4tme4u94c8q3la04gne5ft3\",\"memo\":null}"

In go implementation, the packet data doesn't have an Optional field.
Because ibc-rs reuses this definition, the decoding packet data failed.
When adding #[serde(skip_serializing_if = "Option::is_none")] to the optional members, no null appeared.

Second, shouldn't the data structure compatible with ICS-721 spec? Decoding the above classData in the packet from the contract:

{
  "owner": "wasm1ejs8u53du38wy2ukrpf00nv9r7539w9e6ryfn3",
  "contract_info": {
    "code_id": 1,
    "creator": "wasm1ejs8u53du38wy2ukrpf00nv9r7539w9e6ryfn3",
    "admin": null,
    "pinned": false,
    "ibc_port": null
  },
  "name": "test_nft",
  "symbol": "TEST_NFT",
  "num_tokens": 1
}

ICS-721 spec says:

Both tokenData entries and classData MUST be Base64 encoded strings which SHOULD have the following JSON structure:

{
  "key1" : { "value":"...", "mime":"..." },
  "key2" : { "value":"...", "mime":"..." },
  ...
}

Please correct me if I misinterpret it. Thanks!

if this lands: CosmWasm/cw-nfts#104

Probably nothing.

We'd like to be able to incentivize NFT's entering Stargaze, and require a tax for NFTs exiting Stargaze.

Entry Inventives

1. A subDAO controls what collections should be incentivized.
2. If a NFT from an incentivized collections enters Stargaze, an entry incentives contract sends funds to the NFT receiver.

Exit Incentives

First, funds are deposited with the exit tax module. Then, the NFT is transferred via the exit contract which serves as a proxy for the ICS-721 contract. The module being a proxy means that it is the only module allowed to perform NFT transfers, thus making the exit tax required. The two messages needed can be abstracted by a frontend which sends both messages in one transaction.

In this flowchart, the exit tax is paid even if the NFT transfer fails (ACK-FAIL, or timeout is returned). We could easily extend this so that the exit proxy receives a transfer completion callback (which we'll add as part of integrating with outposts), and thus allow refunding the exit tax if the transfer fails.

Safety

In order for this system to be safe:

1. The exit tax must be larger than the entry inventive to prevent looping.
2. The entry tax must be limited to an allow-list of NFT collections, as otherwise a permissionless chain could instantiate and transfer infinite NFTs to drain the entry incentives.

This hasnt been considered yet for ICS721 version one, but for upcoming release.

Standard cw721 is designed by having onchain metadatat called extension here: https://github.com/CosmWasm/cw-nfts/blob/main/packages/cw721/src/query.rs#L126-L133

pub struct NftInfoResponse<T> {
    /// Universal resource identifier for this NFT
    /// Should point to a JSON file that conforms to the ERC721
    /// Metadata JSON Schema
    pub token_uri: Option<String>,
    /// You can add any custom metadata here when you extend cw721-base
    pub extension: T,
}

ICS721 allows optional token metadata (token_data) to be transferred as binary. So on receival we just need to unwrap and check whether it matchs with NftInfoResponse struct. If so we can store onchain metadata.

Currently, ICS721 stores cw721-base code id, here we need another code id for cw721-metadata-onchain. Then we're good to go.

Currently only success case is handled: https://github.com/public-awesome/ics721/blob/main/contracts/cw-ics721-bridge/src/contract.rs#L266

In case ics721 contract is instantiated with wrong CW721_CODE_ID, during transfer this ack fail is returned:

{"error":"codespace: wasm, code: 4"}

SDK error with code 4 means unauthorized.

In additional pls also have a look here: https://github.com/public-awesome/ics721/blob/main/contracts/cw-ics721-bridge/src/contract.rs#L39

    let proxy_instantiate = msg
        .proxy
        .map(|m| m.into_wasm_msg(env.contract.address))
        .map(|wasm| SubMsg::reply_on_success(wasm, INSTANTIATE_PROXY_REPLY_ID))
        .map_or(vec![], |s| vec![s]);

SG-721 has to account for locked tokens, preventing them from being transferred.

1. Store a map of locked token ids
2. Override send and transfer to disallow working for locked tokens

As discussed here, I had null values in the encoded NonFungibleTokenPacketData. We can opt-out them by #[serde(skip_serializing_if = "Option::is_none")].

{
  "classId": "wasm14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9s0phg4d",
  "classUri": null,
  "classData": "eyJvd25lciI6Indhc20xNHpwMzVhdzl3Y256dGxoazUycGpjbHI2enJ2bXJja3pldXJhNWMiLCJjb250cmFjdF9pbmZvIjp7ImNvZGVfaWQiOjEsImNyZWF0b3IiOiJ3YXNtMTR6cDM1YXc5d2NuenRsaGs1MnBqY2xyNnpydm1yY2t6ZXVyYTVjIiwiYWRtaW4iOm51bGwsInBpbm5lZCI6ZmFsc2UsImliY19wb3J0IjpudWxsfSwibmFtZSI6InRlc3RfbmZ0Iiwic3ltYm9sIjoiVEVTVF9ORlQiLCJudW1fdG9rZW5zIjoxfQ==",
  "tokenIds": [
    "test_nft_0"
  ],
  "tokenUris": [
    "http://example.com"
  ],
  "tokenData": null,
  "sender": "wasm14zp35aw9wcnztlhk52pjclr6zrvmrckzeura5c",
  "receiver": "tnam1qq485cszjczr3379p4tme4u94c8q3la04gne5ft3",
  "memo": null
}

@@ -14,9 +14,11 @@ pub struct NonFungibleTokenPacketData {
     pub class_id: ClassId,
     /// Optional URL that points to metadata about the
     /// collection. Must be non-empty if provided.
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub class_uri: Option<String>,
     /// Optional base64 encoded field which contains on-chain metadata
     /// about the NFT class. Must be non-empty if provided.
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub class_data: Option<Binary>,
     /// Uniquely identifies the tokens in the NFT collection being
     /// transfered. This MUST be non-empty.
@@ -25,11 +27,13 @@ pub struct NonFungibleTokenPacketData {
     /// transfered. `tokenUris[N]` should hold the metadata for
     /// `tokenIds[N]` and both lists should have the same if
     /// provided. Must be non-empty if provided.
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub token_uris: Option<Vec<String>>,
     /// Optional base64 encoded metadata for the tokens being
     /// transfered. `tokenData[N]` should hold metadata for
     /// `tokenIds[N]` and both lists should have the same length if
     /// provided. Must be non-empty if provided.
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub token_data: Option<Vec<Binary>>,

     /// The address sending the tokens on the sending chain.
@@ -38,6 +42,7 @@ pub struct NonFungibleTokenPacketData {
     /// chain.
     pub receiver: String,
     /// Memo to add custom string to the msg
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub memo: Option<String>,
 }

also replace then in package.json and test.sh