receive callback fix: query NFT contract from storage (and not instantiate2) #98

taitruong · 2024-05-31T21:14:06Z

UPDATE: most code changes was done in e2e tests. Huge shoutout to @magiodev!
code for fix is quite small. We felt the need to refactor e2e tests and add more and deeper tests using Go.

For receive callback, nft contract needs to be passed. Before this fix, it was using instantiated2 for retrieving nft contract. As part of instantiate2 it uses cw721's code id for predicting nft contract address:

            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)

But if cw721 code id changes, then it returns wrong address. In this case whole TX fails and NFT gets reverted/returned to sender.

Fix is getting address from CLASS_ID_AND_NFT_CONTRACT_INFO storage:

            Ok(CLASS_ID_AND_NFT_CONTRACT_INFO
                .load(deps.storage, &local_class_id)?
                .address)

taitruong · 2024-05-31T21:39:21Z

packages/ics721/src/ibc_packet_receive.rs

+            // here we know NFT contract is always instantiated, so it is safe getting it from storage
+            // NEVER use instantiate2 to get the NFT contract address here, since code id may change!
+            Ok(CLASS_ID_AND_NFT_CONTRACT_INFO
+                .load(deps.storage, &local_class_id)?
+                .address)


This is the bug fix. Before it used instantiate2 for retrieving nft contract, but in case cw721 code id has changed, it returns an non-existent, unknown address.

Code before:

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)

Why did we use salt to get address before? How to guarantee contract will always exist on retrieval?

Callback happens after collection has been instantiated. ICS721 instantiates it and stores it in CLASS_ID_AND_NFT_CONTRACT_INFO.

salt is required and part of instantiate2. Using class id as salt guarantees that address always results in same escrowed nft contract (predictable). This is used durint instantiation of contract, which then stores predictable address in CLASS_ID_AND_NFT_CONTRACT_INFO. CLASS_ID_AND_NFT_CONTRACT_INFO is then used by mint and callbacks.

Ah yeah so for instantiate2 we need the salt, but here we are now using local_class_id to retrieve from CLASS_ID_AND_NFT_CONTRACT_INFO. Just wondering where do we get local_class_Id from? Seems to be passed into the function.

class_id is passed from source to target chain (e.g. nft contract stars1xyz) as part pf , then target chain attaches destination port and channel e.g. wasm.osmo1ics721/channel-xyz/stars1xyz). This is stored as key and nft contract as value in CLASS_ID_AND_NFT_CONTRACT_INFO map.

taitruong · 2024-05-31T21:40:54Z

packages/ics721/src/msg.rs

+    #[returns(::cosmwasm_std::Addr)]
+    GetInstantiate2NftContract {
+        class_id: String,
+        cw721_code_id: Option<u64>,
+    },


Added new GetInstantiate2NftContract, which will be super helpful - e.g. for WLing collections without doing an initial transfer!

taitruong · 2024-05-31T22:23:10Z

packages/ics721/src/execute.rs

+        let (class_id_info, instantiate) =
+            self.create_instantiate_msg(deps, &env, class.clone())?;
+
+        let token_ids = format!("{:?}", tokens);
+        let event = Event::new("ics721_receive_create_vouchers")
+            .add_attribute("class_id", class_id_info.class_id)
+            .add_attribute("nft_contract", class_id_info.address)
+            .add_attribute("token_ids", token_ids);


Added 3 new events, for better processing (e.g. for constellations):

event when NFTs are created/minted...

taitruong · 2024-05-31T22:23:52Z

packages/ics721/src/execute.rs

+        let token_ids_string = format!("{:?}", token_ids);
+        let event = Event::new("ics721_receive_redeem_vouchers")
+            .add_attribute("class_id", class.id)
+            .add_attribute("nft_contract", nft_contract.clone())
+            .add_attribute("token_ids", token_ids_string);


event when NFTs are unescrowed/returned...

taitruong · 2024-05-31T22:24:20Z

packages/ics721/src/ibc.rs

+            let token_ids = format!("{:?}", msg.token_ids);
+            let event = Event::new("ics721_ack_burn_vouchers")
+                .add_attribute("class_id", msg.class_id.to_string())
+                .add_attribute("nft_contract", nft_contract.clone())
+                .add_attribute("token_ids", token_ids.clone());


event when NFTs are burned

Art3miX

Just 2 mid-ish issues.

Art3miX · 2024-06-03T10:18:08Z

packages/ics721/src/ibc_packet_receive.rs

-            let salt = hasher.finalize().to_vec();
-
-            get_instantiate2_address(deps, env.contract.address.as_str(), &salt, cw721_code_id)
+            // here we know NFT contract is always instantiated, so it is safe getting it from storage


Do we?
Because CLASS_ID_AND_NFT_CONTRACT_INFO storage is only updated on the create_voucher_and_channel_messages, which is executed AFTER create_callback_msg, meaning when we try to load from storage here, its still might be missing which should panic, on first transferred NFT.

We should check here if we have it in storage, get it, if we don't, get the init2 of it, which works because if we don't have it storage, it means we are creating it right now, which means we use the correct code id for it.

Art3miX · 2024-06-03T10:21:57Z

packages/ics721/src/query.rs

@@ -88,6 +96,28 @@ pub fn query_nft_contract_for_class_id(
        .map(|e| e.map(|(_, v)| v.address))
 }

+pub fn query_get_nft_contract_for_class_id(


Would change the name to reflect that it uses init2 (which might not be the actual address if wrong info is given).

Also why we even need it? we have the storage, if the contract doesn't exists in storage, means we didn't init it yet, and we can use init2 with the saved code in storage.

Art3miX · 2024-06-06T09:00:11Z

packages/ics721/src/ibc_packet_receive.rs

+            let nft_contract =
+                match query_nft_contract_for_class_id(deps.storage, local_class_id.clone()) {
+                    Ok(nft_contract) => nft_contract,
+                    Err(_) => None, // not found, occurs on initial transfer when we don't have the contract address
+                };
+            match nft_contract {


match query_nft_contract_for_class_id(deps.storage, local_class_id.clone()).ok_or(None), can this work?

…1BaseCodeId

taitruong commented May 31, 2024

View reviewed changes

taitruong changed the title ~~Class id fix~~ fix: query NFT contract from storage (and not instantiate2) May 31, 2024

taitruong added 4 commits June 1, 2024 00:32

new GetNftContractByInstantiate2

dd6d788

fix nft contract retrieval using CLASS_ID_AND_NFT_CONTRACT_INFO

917512b

cleanup

4f228d3

add events

bfe24cb

taitruong force-pushed the class_id_fix branch from b204367 to bfe24cb Compare May 31, 2024 22:32

taitruong changed the title ~~fix: query NFT contract from storage (and not instantiate2)~~ receive callback fix: query NFT contract from storage (and not instantiate2) May 31, 2024

taitruong requested review from shanev, jhernandezb, humanalgorithm and Art3miX June 1, 2024 07:09

humanalgorithm approved these changes Jun 1, 2024

View reviewed changes

Art3miX requested changes Jun 3, 2024

View reviewed changes

taitruong added 2 commits June 5, 2024 15:04

cleanup

04acd23

use instantiate2 in case addr not in storage

4861d1b

Art3miX approved these changes Jun 6, 2024

View reviewed changes

taitruong mentioned this pull request Jun 10, 2024

go-based e2e testing for callbacks and cw721 backward compatibility #101

Open

taitruong force-pushed the class_id_fix branch from 85f4005 to df07fd0 Compare June 10, 2024 18:33

log more attributes (nft_contract and is_redemption)

ceb3a29

taitruong force-pushed the class_id_fix branch from df07fd0 to ceb3a29 Compare June 10, 2024 18:33

taitruong and others added 7 commits June 10, 2024 21:39

fix test

315aaee

refactor e2e test suite

ed87301

reintroduce init.go to override gas

d4c3142

passing tests

8acf6f2

polish basic_test

6ce349f

ignored artifacts for cw721 v0.16 and v0.17

4b568e1

generalize SendIcsFromChainToChain function

58ff73e

magiodev added 6 commits June 20, 2024 12:01

nft to nftContract as string

58b0e19

callback test

10e9c02

test transfers with migrate with_update in between

fae299e

rename CW721CodeID to Cw721BaseCodeId for InstantiateICS721Bridge struct

944682c

rename functions

af2c603

callback class_id change test migrating with update and changing Cw72…

811f567

…1BaseCodeId

humanalgorithm approved these changes Jun 20, 2024

View reviewed changes

shanev approved these changes Jun 20, 2024

View reviewed changes

shanev added this pull request to the merge queue Jun 20, 2024

Merged via the queue into public-awesome:main with commit 8e8de23 Jun 20, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

receive callback fix: query NFT contract from storage (and not instantiate2) #98

receive callback fix: query NFT contract from storage (and not instantiate2) #98

taitruong commented May 31, 2024 •

edited

Loading

taitruong May 31, 2024 •

edited

Loading

humanalgorithm Jun 1, 2024

taitruong Jun 1, 2024

humanalgorithm Jun 1, 2024

taitruong Jun 1, 2024

taitruong May 31, 2024 •

edited

Loading

taitruong May 31, 2024 •

edited

Loading

taitruong May 31, 2024

taitruong May 31, 2024

Art3miX left a comment

Art3miX Jun 3, 2024

Art3miX Jun 3, 2024

Art3miX Jun 6, 2024

receive callback fix: query NFT contract from storage (and not instantiate2) #98

receive callback fix: query NFT contract from storage (and not instantiate2) #98

Conversation

taitruong commented May 31, 2024 • edited Loading

taitruong May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taitruong May 31, 2024 • edited Loading

Choose a reason for hiding this comment

taitruong May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Art3miX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taitruong commented May 31, 2024 •

edited

Loading

taitruong May 31, 2024 •

edited

Loading

taitruong May 31, 2024 •

edited

Loading

taitruong May 31, 2024 •

edited

Loading