instantiate2 by taitruong · Pull Request #71 · public-awesome/cw-ics721

taitruong · 2023-11-11T15:59:40Z

No description provided.

taitruong · 2023-11-11T16:03:43Z

        let instantiate = if CLASS_ID_TO_NFT_CONTRACT.has(deps.storage, class.id.clone()) {
            vec![]
        } else {
+            let class_id = ClassId::new(class.id.clone());


part 1 of instantiate2: using class_id as salt,

taitruong · 2023-11-11T16:06:30Z

-                CLASS_ID_TO_NFT_CONTRACT.save(deps.storage, class_id.clone(), &cw721_addr)?;
-                NFT_CONTRACT_TO_CLASS_ID.save(deps.storage, cw721_addr.clone(), &class_id)?;
+                // cw721 addr has already been stored, here just check whether it exists, otherwise a NotFound error is thrown
+                let class_id = NFT_CONTRACT_TO_CLASS_ID.load(deps.storage, cw721_addr.clone())?;


part 3, no need storing class id in name and symbol, instead: once instantiated by sub msg, query, whether return addr fits with addr created before using instantiate2.

taitruong · 2023-11-11T16:09:24Z

+#[derive(Default)]
+pub struct MockAddressGenerator;
+
+impl AddressGenerator for MockAddressGenerator {


biggest changes are in test, using MockAddressGenerator and MockApiBech32. In generator, predictable_contract_address() function covers instantiate2.

Also all tests had to be refactored and cleanup for better readability.

humanalgorithm · 2023-11-12T08:25:45Z

+                    }
+                }
+                // set name and symbol
+                instantiate_msg.symbol = class_data.symbol;


nice looks like you can take name and symbol right from the class data object?

Some background here: ics721 need to store class id <> nft contract map entries (e.g. for determining incoming NFTs are back- (=burn) or forward transfer (=mint).

Before usage of instantiate2 we had to store class id in collection, since address is unknown yet. and on instantiation reply we have address but no class id, so had to store it as part of collection.
Which led to the side effect frontends not having source name and symbol.

By using instantiate2 ics721 now knows address - even before instantiation. So it can store name and symbol provided by source chain.

humanalgorithm · 2023-11-12T08:27:48Z

+            .iter()
+            .map(|part| part.trim().parse().unwrap())
+            .collect();
+        let canonical_addr = instantiate2_address(checksum, creator, &salt)?;


the magic is here

Correct, this is executed when WasmMsg::Instantiate2 is called.

humanalgorithm · 2023-11-12T08:34:45Z

+
            let message = SubMsg::<T>::reply_on_success(
-                WasmMsg::Instantiate {
+                WasmMsg::Instantiate2 {


This is the main message that we are trying to get back in this PR right?

…d make sure instantiate2 creates 2 different, predictable contracts

humanalgorithm · 2023-11-15T02:58:13Z

+            let CodeInfoResponse { checksum, .. } = deps
+                .querier
+                .query_wasm_code_info(CW721_CODE_ID.load(deps.storage)?)?;
+            let canonical_cw721_addr = instantiate2_address(&checksum, &canonical_creator, &salt)?;


Can get a quick explanation of what is going on here?

This is mainly explained in docs here: https://github.com/CosmWasm/cosmwasm/blob/main/packages/std/src/addresses.rs#L270-L309

Essentially it uses salt for creating a unique address. Since class_id is unique for ics721, it is an ideal candidate for salt. NFT module has a exactly same approach: instantiated nft contract is a hash of class id.

humanalgorithm · 2023-11-15T03:03:55Z

+        else {
+            // Token metadata is set unconditionaly on mint. If we have no
+            // metadata entry, we have no entry for this token at all.
+            return Ok(None);


If there is no token metadata we just return a None. Does the caller see that as an error? What does Ok(None) mean to the contract that is calling this?

ics721 has prepared for handling token metadata. prepared means it is stored in ics721 rn - since core cw721 package does not support it per se. see zeke's comment in callback_mint() in execute.rs:

fn callback_mint( &self, deps: DepsMut, class_id: ClassId, tokens: Vec<Token>, receiver: String, ) -> Result<Response<T>, ContractError> { let receiver = deps.api.addr_validate(&receiver)?; let cw721_addr = CLASS_ID_TO_NFT_CONTRACT.load(deps.storage, class_id.clone())?; let mint = tokens .into_iter() .map(|Token { id, uri, data }| { // We save token metadata here as, ideally, once cw721 // supports on-chain metadata, this is where we will set // that value on the debt-voucher token. Note that this is // set for every token, regardless of if data is None. TOKEN_METADATA.save(deps.storage, (class_id.clone(), id.clone()), &data)?; ...

Flow is like this:

on send from source, metadata is loaded (which is currently None)

pass to target as part of NonFungibleTokenPacketData

save metadata in ics721 on target chain

I just want to be clear here, so ics721. is storing onchain metadata on ics721? As in, when someone moves their nft from A to B, metadata is saved on chain B's ics721 contract. In our original discussion of this issue, we talked about querying original chain to get the onchain metadata from its original home chain. Does this mean we have abandoned that method?

No, we didn't abandoned it. What we store here is on initial transfer and storing data provided from NonFungibleTokenPacketData. Also added another commit with some more comments on metadata. Pls check also my other 2 comments on TOKEN_METADATA. Please note, this is not part of this PR - only added some comments in source for clarifications.

humanalgorithm · 2023-11-15T03:07:58Z

 # specified in this config (e.g. 0.25token1;0.0001token2).
 minimum-gas-prices = "0uosmo"

+# The maximum gas a query coming over rest/grpc may consume.


Why we have an Osmosis script in ics721?

we use ts-relayer for integration tests - provided by confio. their template is here: https://github.com/confio/cw-ibc-demo/
it provides 3 chains for integration tests: wasmd, osmosis and gaia. first 2 supports cosmwasm, whilst gaia does not. in repo it also contains genesis data for each chain - making it easy to setup tests.
For maintenance it is better using these default chains - otherwise we have to maintain them on our own. docker images can be found in confio/relayer-ci-images repo.

Did also a PR on upgrading to wasmd 0.31 here: confio/relayer-ci-images#3

Would be great if SG contributes a local SG with base genesis setup. This way ICS721 could also cover specific SG tests.

Nice, fyi @jhernandezb

taitruong · 2023-11-15T11:28:07Z

+        // here NFT is transferred to another chain, NFT itself may have been transferred to his chain before
+        // in this case ICS721 may have metadata stored
        let token_metadata = TOKEN_METADATA
            .may_load(deps.storage, (class.id.clone(), token_id.clone()))?


during receive_nft() (starting point from nft contract to ics721 for transfer) read TOKEN_METADATA (in case it has been provided on previous, initial transfer to this chain), this is used for transferring to next chain. Not part of this PR. just outlining what happens here.

taitruong · 2023-11-15T11:31:04Z

+                // Source chain may have provided token metadata, so we save token metadata here
+                // Note, once cw721 doesn't support on-chain metadata yet - but this is where we will set
+                // that value on the debt-voucher token once it is supported.
+                // Also note that this is set for every token, regardless of if data is None.


This TOKEN_METADATA part is on receival on target chain, here we store metadata, for later re-transfer. This is not used on mint, since cw721 doesnt support on-chain metadata (yet).

taitruong added 6 commits October 15, 2023 23:38

use cosmwasm_std::Never

ab3aa2f

first draft using instantiate2

474b1fa

fix build error, enable cosmwasm_1_2 feature for using instantiate2

3d35065

cleanup, cargo clippy, replaced deprecated functions

c0aa8dc

first draft: get name and symbol from class data

6f86af1

set creator, name, and symbol for sg-ics721

7e1912e

taitruong commented Nov 11, 2023

View reviewed changes

humanalgorithm reviewed Nov 12, 2023

View reviewed changes

Comment thread contracts/sg-ics721/src/testing/integration_tests.rs

humanalgorithm reviewed Nov 12, 2023

View reviewed changes

taitruong added 2 commits November 14, 2023 13:35

fix salt

b8c63f0

upgrade to latest ts-relayer version

3c88d5c

taitruong force-pushed the main branch from 6f31cff to 3c88d5c Compare November 14, 2023 15:01

taitruong added 5 commits November 14, 2023 16:13

update to wasmd 0.31, create new genesis data

0a95563

fix class id, using sha267 guarantees salt is always of lenght 32

4010f53

use real life class data for testig salt

9db0c66

test case: instantiate two cw721 contracts with different class id an…

f36d175

…d make sure instantiate2 creates 2 different, predictable contracts

fix e2e test

5e19c41

humanalgorithm reviewed Nov 15, 2023

View reviewed changes

more description on TOKEN_METADATA

f01048b

taitruong commented Nov 15, 2023

View reviewed changes

shanev approved these changes Nov 16, 2023

View reviewed changes

jhernandezb approved these changes Nov 17, 2023

View reviewed changes

humanalgorithm self-assigned this Nov 17, 2023

humanalgorithm self-requested a review November 17, 2023 06:58

humanalgorithm approved these changes Nov 17, 2023

View reviewed changes

humanalgorithm added this pull request to the merge queue Nov 17, 2023

Merged via the queue into public-awesome:main with commit 4fe21cc Nov 17, 2023

taitruong mentioned this pull request Nov 17, 2023

name and symbol #70

Closed

Uh oh!

Conversation

taitruong commented Nov 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants