Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AppBuilder::new().with_api(): how to override default api? #95

Closed
taitruong opened this issue Nov 1, 2023 · 10 comments
Closed

AppBuilder::new().with_api(): how to override default api? #95

taitruong opened this issue Nov 1, 2023 · 10 comments
Assignees

Comments

@taitruong
Copy link

taitruong commented Nov 1, 2023

Hi, couldn't find an example of how default API could be overridden. Reason is because of this:

MockApi's addr_humanize incompatible with instantiate2_address
CosmWasm/cosmwasm#1648

Problem is in MockApi: https://github.com/CosmWasm/cosmwasm/blob/fde26bd6293aaacb901c3fb5a20de0d89c339b38/packages/vm/src/testing/mock.rs#L132-L139

I know how to override wasm executor using AppBuilder::new().with_wasm() - maybe with_api() is similar to this:

#[derive(Debug)]
struct Bech32AddressGenerator {
    pub hrp: String,
}

const COUNT: Item<u8> = Item::new("count");

impl Bech32AddressGenerator {
    pub const fn new(hrp: String) -> Self {
        Self { hrp }
    }
}

impl AddressGenerator for Bech32AddressGenerator {
    fn next_address(&self, storage: &mut dyn Storage) -> Addr {
        let count = match COUNT.may_load(storage) {
            Ok(Some(count)) => count,
            _ => 0,
        };
        let data = bech32::u5::try_from_u8(count).unwrap();
        let encoded_addr = bech32::encode(self.hrp.as_str(), vec![data], Variant::Bech32).unwrap();
        let addr = Addr::unchecked(encoded_addr);
        COUNT.save(storage, &(count + 1)).unwrap();
        addr
    }
}

impl Test {
    fn new(proxy: bool, pauser: Option<String>, cw721_code: Box<dyn Contract<Empty>>) -> Self {
        let mut app = AppBuilder::new()
            .with_wasm::<FailingModule<Empty, Empty, Empty>, WasmKeeper<Empty, Empty>>(
                WasmKeeper::new_with_custom_address_generator(Bech32AddressGenerator::new(
                    TARGET_HRP.to_string(),
                )),
            )
            .with_ibc(IbcAcceptingModule)
            .build(no_init);
...
@webmaster128
Copy link
Member

There is already with_api. So what you'd need to do is to create your own version of MockApi by implementing the cosmwasm_std::Api trait. You could e.g. make MyApi with impl cosmwasm_std::Api for MyApi and then you forward all API calls to a MockApi except for the address stuff. This way you don't have to implement the crypro yourself.

@webmaster128
Copy link
Member

There is even MockApiBech32 in this repo

@taitruong
Copy link
Author

There is even MockApiBech32 in this repo

Searched and didn't find MockApiBech32

@taitruong
Copy link
Author

There is already with_api. So what you'd need to do is to create your own version of MockApi by implementing the cosmwasm_std::Api trait. You could e.g. make MyApi with impl cosmwasm_std::Api for MyApi and then you forward all API calls to a MockApi except for the address stuff. This way you don't have to implement the crypro yourself.

Actually I'm trying to test instantiate2_address, and maybe problem is somewhere else? Instantiate2 seems not to be mocked, it returns me this for example: CanonicalAddr(Binary(664d735b8a8cb7b97603e805b355a16c72227cd50ac403dfc819db46c45c5f28))

My Api implementation for addr_canonicalize() just wraps human string into Binary: CanonicalAddr(to_binary(&normalized)?), and vice versa for addr_humanize(), I just unwraps it: let human: String = from_binary(&canonical.0)?;

So, is it possible to mock instantiate2 as well? This way I can just create another CanonicalAddr with string binary.

@DariuszDepta
Copy link
Member

Hi all,
the example of using Bech32 in tests is shown in test file here:
https://github.com/CosmWasm/cw-multi-test/blob/main/tests/test_app_builder/test_with_api.rs

It uses MockApiBech32 defined also in tests:
https://github.com/CosmWasm/cw-multi-test/blob/main/tests/mod.rs

These two files show how to replace Api to handle Bech32 addresses.

But for generating contract addresses in Bech32 format, the AddressGenerator is needed.
And an example of such generator is defined also here in tests:

cw-multi-test/tests/mod.rs

Lines 185 to 230 in beb8ad1

pub struct MockAddressGenerator;
impl AddressGenerator for MockAddressGenerator {
fn contract_address(
&self,
api: &dyn Api,
_storage: &mut dyn Storage,
code_id: u64,
instance_id: u64,
) -> AnyResult<Addr> {
let canonical_addr = Self::instantiate_address(code_id, instance_id);
Ok(Addr::unchecked(api.addr_humanize(&canonical_addr)?))
}
fn predictable_contract_address(
&self,
api: &dyn Api,
_storage: &mut dyn Storage,
_code_id: u64,
_instance_id: u64,
checksum: &[u8],
creator: &CanonicalAddr,
salt: &[u8],
) -> AnyResult<Addr> {
let canonical_addr = instantiate2_address(checksum, creator, salt)?;
Ok(Addr::unchecked(api.addr_humanize(&canonical_addr)?))
}
}
impl MockAddressGenerator {
// non-predictable contract address generator, see `BuildContractAddressClassic`
// implementation in wasmd: https://github.com/CosmWasm/wasmd/blob/main/x/wasm/keeper/addresses.go#L35-L42
fn instantiate_address(code_id: u64, instance_id: u64) -> CanonicalAddr {
let mut key = Vec::<u8>::new();
key.extend_from_slice(b"wasm\0");
key.extend_from_slice(&code_id.to_be_bytes());
key.extend_from_slice(&instance_id.to_be_bytes());
let module = Sha256::digest("module".as_bytes());
Sha256::new()
.chain(module)
.chain(key)
.finalize()
.to_vec()
.into()
}
}

I hope this helps ;-)

@taitruong
Copy link
Author

taitruong commented Nov 9, 2023

Hi all, the example of using Bech32 in tests is shown in test file here: https://github.com/CosmWasm/cw-multi-test/blob/main/tests/test_app_builder/test_with_api.rs

It uses MockApiBech32 defined also in tests: https://github.com/CosmWasm/cw-multi-test/blob/main/tests/mod.rs

These two files show how to replace Api to handle Bech32 addresses.

But for generating contract addresses in Bech32 format, the AddressGenerator is needed. And an example of such generator is defined also here in tests:

cw-multi-test/tests/mod.rs

Lines 185 to 230 in beb8ad1

pub struct MockAddressGenerator;
impl AddressGenerator for MockAddressGenerator {
fn contract_address(
&self,
api: &dyn Api,
_storage: &mut dyn Storage,
code_id: u64,
instance_id: u64,
) -> AnyResult<Addr> {
let canonical_addr = Self::instantiate_address(code_id, instance_id);
Ok(Addr::unchecked(api.addr_humanize(&canonical_addr)?))
}
fn predictable_contract_address(
&self,
api: &dyn Api,
_storage: &mut dyn Storage,
_code_id: u64,
_instance_id: u64,
checksum: &[u8],
creator: &CanonicalAddr,
salt: &[u8],
) -> AnyResult<Addr> {
let canonical_addr = instantiate2_address(checksum, creator, salt)?;
Ok(Addr::unchecked(api.addr_humanize(&canonical_addr)?))
}
}
impl MockAddressGenerator {
// non-predictable contract address generator, see `BuildContractAddressClassic`
// implementation in wasmd: https://github.com/CosmWasm/wasmd/blob/main/x/wasm/keeper/addresses.go#L35-L42
fn instantiate_address(code_id: u64, instance_id: u64) -> CanonicalAddr {
let mut key = Vec::<u8>::new();
key.extend_from_slice(b"wasm\0");
key.extend_from_slice(&code_id.to_be_bytes());
key.extend_from_slice(&instance_id.to_be_bytes());
let module = Sha256::digest("module".as_bytes());
Sha256::new()
.chain(module)
.chain(key)
.finalize()
.to_vec()
.into()
}
}

I hope this helps ;-)

Absolute awsome, that helps! I've basically copied ´MockAddressGeneratorandMockApiBech32`. In my code I use a string as a salt:

let salt = "bad kids".as_bytes();
let canonical_creator = deps.api.addr_canonicalize(env.contract.address.as_str())?;
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)
                .map_err(|_| StdError::generic_err("Could not calculate addr"))?;

So for "bad kids", the bytes array is: 98,97,100,32,107,105,100,115. Now in MockAddressGenerator.predictable_contract_address() I have copied this code:

fn predictable_contract_address(
    &self,
    api: &dyn Api,
    _storage: &mut dyn Storage,
    _code_id: u64,
    _instance_id: u64,
    checksum: &[u8],
    creator: &CanonicalAddr,
    salt: &[u8],
) -> AnyResult<Addr> {
    let canonical_addr = instantiate2_address(checksum, creator, salt)?;
    Ok(Addr::unchecked(api.addr_humanize(&canonical_addr)?))
}

But interestingly, when contract is instantiated, the salt here is: 39, 9, 91, 67, 143, 112, 174, 211, 84, 5, 20, 155, 197, 232, 223, 161, 212, 97, 247, 205, 156, 37, 53, 152, 7, 173, 102, 220, 193, 57, 111, 199

Which is the string representation for [98,97,100,32,107,105,100,115]!

So I had to adjust predictable_contract_address() by this:

  • convert salt to string representation
  • remove brackets and commas from string representation
  • convert back to salt
fn predictable_contract_address(
    &self,
    api: &dyn Api,
    _storage: &mut dyn Storage,
    _code_id: u64,
    _instance_id: u64,
    checksum: &[u8],
    creator: &CanonicalAddr,
    salt: &[u8],
) -> Result<Addr> {
    // string representation of the salt
    let salt_to_string = std::str::from_utf8(salt)?;
    // Remove the square brackets and split the string by commas
    let parts: Vec<&str> = salt_to_string.trim_matches(|c| c == '[' || c == ']').split(',').collect();
    // Convert each part to a u8 and collect them into a Vec<u8>
    let salt: Vec<u8> = parts.iter().map(|part| part.trim().parse().unwrap()).collect();
    let canonical_addr = instantiate2_address(checksum, creator, &salt)?;
    Ok(Addr::unchecked(api.addr_humanize(&canonical_addr)?))
}

Tests are now running, but not sure whether above really solves the problem or it is just a hack for root of cause - which I haven't found yet?!?

@DariuszDepta
Copy link
Member

Hi @taitruong, unfortunately I do not get the same effect as you with the salt.
I have prepared a test case that uses both MockApiBech32 and MockAddressGenerator toegether,
and the salt passed to MockAddressGenerator.predictable_contract_address is exactly as expected:
[98, 97, 100, 32, 107, 105, 100, 115] ("bad kids" as bytes).

Could you take a look at this PR #96, maybe I am missing something that is relevant in your test case?

Thanks in advance!

@DariuszDepta DariuszDepta self-assigned this Nov 13, 2023
@taitruong
Copy link
Author

Hi @taitruong, unfortunately I do not get the same effect as you with the salt. I have prepared a test case that uses both MockApiBech32 and MockAddressGenerator toegether, and the salt passed to MockAddressGenerator.predictable_contract_address is exactly as expected: [98, 97, 100, 32, 107, 105, 100, 115] ("bad kids" as bytes).

Could you take a look at this PR #96, maybe I am missing something that is relevant in your test case?

Thanks in advance!

I will - as soon as I'm done with some other tasks. In the meanwhile you can check my code here: https://github.com/public-awesome/cw-ics721/pull/71/files#diff-4aa2f03efbf149b4f455e1d534278edc33135c23cd29d2ad7f8e46b74eaaa39d

In packages/ics721/src/testing/integration_tests.rs you can find my workaround in predictable_contract_address().

@DariuszDepta
Copy link
Member

Hi @taitruong, I guess I have spotted the issue. There is a conversion from [u8] into JSON binary for a salt:

salt: to_json_binary(salt)?,

which converts to binary representation of the text, so you get the effect you have described before. Please change it to:

salt: salt.into(),

It is enough to convert byte array to Binary in this case, if you want to keep the salt in the binary form, of course. I have prepared a diff patch from your branch to point the places to be changed. Generally please change as described above and remove the salt hacking in MockAddressGenerator, and it should work.

After applying the changes, calling:

$ cargo clean
$ cargo build
$ cargo clippy
$ cargo test

worked fine for me.

If this solves the issue, please let me know if I can close it. Thanks!

Git diff:

diff --git a/contracts/sg-ics721/src/testing/integration_tests.rs b/contracts/sg-ics721/src/testing/integration_tests.rs
index 495df03..2862c74 100644
--- a/contracts/sg-ics721/src/testing/integration_tests.rs
+++ b/contracts/sg-ics721/src/testing/integration_tests.rs
@@ -120,18 +120,6 @@ impl AddressGenerator for MockAddressGenerator {
         creator: &CanonicalAddr,
         salt: &[u8],
     ) -> Result<Addr> {
-        // string representation of the salt
-        let salt_to_string = std::str::from_utf8(salt)?;
-        // Remove the square brackets and split the string by commas
-        let parts: Vec<&str> = salt_to_string
-            .trim_matches(|c| c == '[' || c == ']')
-            .split(',')
-            .collect();
-        // Convert each part to a u8 and collect them into a Vec<u8>
-        let salt: Vec<u8> = parts
-            .iter()
-            .map(|part| part.trim().parse().unwrap())
-            .collect();
         let canonical_addr = instantiate2_address(checksum, creator, &salt)?;
         Ok(Addr::unchecked(api.addr_humanize(&canonical_addr)?))
     }
diff --git a/packages/ics721/src/execute.rs b/packages/ics721/src/execute.rs
index 506c1a5..1cb2abf 100644
--- a/packages/ics721/src/execute.rs
+++ b/packages/ics721/src/execute.rs
@@ -285,7 +285,7 @@ where
                     // can make this field too long which causes data
                     // errors in the SDK.
                     label: "ics-721 debt-voucher cw-721".to_string(),
-                    salt: to_json_binary(salt)?,
+                    salt: salt.into(),
                 },
                 INSTANTIATE_CW721_REPLY_ID,
             );
diff --git a/packages/ics721/src/testing/integration_tests.rs b/packages/ics721/src/testing/integration_tests.rs
index b2c341e..4145f02 100644
--- a/packages/ics721/src/testing/integration_tests.rs
+++ b/packages/ics721/src/testing/integration_tests.rs
@@ -120,18 +120,6 @@ impl AddressGenerator for MockAddressGenerator {
         creator: &CanonicalAddr,
         salt: &[u8],
     ) -> Result<Addr> {
-        // string representation of the salt
-        let salt_to_string = std::str::from_utf8(salt)?;
-        // Remove the square brackets and split the string by commas
-        let parts: Vec<&str> = salt_to_string
-            .trim_matches(|c| c == '[' || c == ']')
-            .split(',')
-            .collect();
-        // Convert each part to a u8 and collect them into a Vec<u8>
-        let salt: Vec<u8> = parts
-            .iter()
-            .map(|part| part.trim().parse().unwrap())
-            .collect();
         let canonical_addr = instantiate2_address(checksum, creator, &salt)?;
         Ok(Addr::unchecked(api.addr_humanize(&canonical_addr)?))
     }

@taitruong
Copy link
Author

Hi @taitruong, I guess I have spotted the issue. There is a conversion from [u8] into JSON binary for a salt:

salt: to_json_binary(salt)?,

which converts to binary representation of the text, so you get the effect you have described before. Please change it to:

salt: salt.into(),

It is enough to convert byte array to Binary in this case, if you want to keep the salt in the binary form, of course. I have prepared a diff patch from your branch to point the places to be changed. Generally please change as described above and remove the salt hacking in MockAddressGenerator, and it should work.

After applying the changes, calling:

$ cargo clean
$ cargo build
$ cargo clippy
$ cargo test

worked fine for me.

If this solves the issue, please let me know if I can close it. Thanks!

Git diff:

diff --git a/contracts/sg-ics721/src/testing/integration_tests.rs b/contracts/sg-ics721/src/testing/integration_tests.rs
index 495df03..2862c74 100644
--- a/contracts/sg-ics721/src/testing/integration_tests.rs
+++ b/contracts/sg-ics721/src/testing/integration_tests.rs
@@ -120,18 +120,6 @@ impl AddressGenerator for MockAddressGenerator {
         creator: &CanonicalAddr,
         salt: &[u8],
     ) -> Result<Addr> {
-        // string representation of the salt
-        let salt_to_string = std::str::from_utf8(salt)?;
-        // Remove the square brackets and split the string by commas
-        let parts: Vec<&str> = salt_to_string
-            .trim_matches(|c| c == '[' || c == ']')
-            .split(',')
-            .collect();
-        // Convert each part to a u8 and collect them into a Vec<u8>
-        let salt: Vec<u8> = parts
-            .iter()
-            .map(|part| part.trim().parse().unwrap())
-            .collect();
         let canonical_addr = instantiate2_address(checksum, creator, &salt)?;
         Ok(Addr::unchecked(api.addr_humanize(&canonical_addr)?))
     }
diff --git a/packages/ics721/src/execute.rs b/packages/ics721/src/execute.rs
index 506c1a5..1cb2abf 100644
--- a/packages/ics721/src/execute.rs
+++ b/packages/ics721/src/execute.rs
@@ -285,7 +285,7 @@ where
                     // can make this field too long which causes data
                     // errors in the SDK.
                     label: "ics-721 debt-voucher cw-721".to_string(),
-                    salt: to_json_binary(salt)?,
+                    salt: salt.into(),
                 },
                 INSTANTIATE_CW721_REPLY_ID,
             );
diff --git a/packages/ics721/src/testing/integration_tests.rs b/packages/ics721/src/testing/integration_tests.rs
index b2c341e..4145f02 100644
--- a/packages/ics721/src/testing/integration_tests.rs
+++ b/packages/ics721/src/testing/integration_tests.rs
@@ -120,18 +120,6 @@ impl AddressGenerator for MockAddressGenerator {
         creator: &CanonicalAddr,
         salt: &[u8],
     ) -> Result<Addr> {
-        // string representation of the salt
-        let salt_to_string = std::str::from_utf8(salt)?;
-        // Remove the square brackets and split the string by commas
-        let parts: Vec<&str> = salt_to_string
-            .trim_matches(|c| c == '[' || c == ']')
-            .split(',')
-            .collect();
-        // Convert each part to a u8 and collect them into a Vec<u8>
-        let salt: Vec<u8> = parts
-            .iter()
-            .map(|part| part.trim().parse().unwrap())
-            .collect();
         let canonical_addr = instantiate2_address(checksum, creator, &salt)?;
         Ok(Addr::unchecked(api.addr_humanize(&canonical_addr)?))
     }

YOU r fantastic. Saved my day! Thank u so much, ser!

@DariuszDepta DariuszDepta mentioned this issue Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants