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

Add test to check custom AddressGenerator implementation #110

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

epanchee
Copy link
Contributor

@epanchee epanchee commented Nov 30, 2023

Hi @DariuszDepta,

This PR contains failing test showing custom address generator is broken. Could you check why custom address generator no longer updated in app builder? Figured this out when was migrating from cw-multi-test:v0.16.5 to the latest version (v0.19.0).

I briefly checked via Clion debugger and it seems that this function actually does update Box pointer here. However, vtable still points to the default address generator implementation. No clue why

@DariuszDepta
Copy link
Member

DariuszDepta commented Dec 1, 2023

Hi @epanchee,

Since version 0.18.0, the function next_address is deprecated and in fact not used anymore for generating contract addresses. I think this may be your case while migrating from 0.16.5 to 0.19.0 of CosmWasm MultiTest.
We will consider better changelog communication to avoid such situations in the future.
Meanwhile, there is a working example based on your test case shown below.
Let me know if it helped.

#[test]
fn custom_address_generator_should_work() {
    struct CustomAddressGenerator;

    impl AddressGenerator for CustomAddressGenerator {
        // deprecated from version 0.18.0
        fn next_address(&self, _storage: &mut dyn Storage) -> Addr {
            Addr::unchecked("deprecated")
        }

        // use this function instead of next_address
        fn contract_address(
            &self,
            _api: &dyn Api,
            _storage: &mut dyn Storage,
            _code_id: u64,
            _instance_id: u64,
        ) -> AnyResult<Addr> {
            Ok(Addr::unchecked("test_address"))
        }
    }

    // prepare wasm module with custom address generator
    let wasm_keeper = WasmKeeper::new().with_address_generator(CustomAddressGenerator);

    let mut app = AppBuilder::default().with_wasm(wasm_keeper).build(no_init);

    // store contract's code
    let code_id = app.store_code(test_contracts::counter::contract());

    let contract_addr = app
        .instantiate_contract(
            code_id,
            Addr::unchecked("owner"),
            &Empty {},
            &[],
            "Counter",
            None,
        )
        .unwrap();

    // make sure that contract address equals to "test_address" generated by custom address generator
    assert_eq!(contract_addr.as_str(), "test_address");
}

Anyway, let us append this test case to MultiTest after fixing, that also other developers can benefit from it.
Would you adjust your PR that I can just approve and merge it?
Thanks in advance!

@epanchee epanchee changed the title Add failing test demonstrating custom address generator is broken Add test to check custom AddressGenerator implementation Dec 1, 2023
@epanchee
Copy link
Contributor Author

epanchee commented Dec 1, 2023

Hi @epanchee,

Since version 0.18.0, the function next_address is deprecated and in fact not used anymore for generating contract addresses. I think this may be your case while migrating from 0.16.5 to 0.19.0 of CosmWasm MultiTest. We will consider better changelog communication to avoid such situations in the future. Meanwhile, there is a working example based on your test case shown below. Let me know if it helped.

#[test]
fn custom_address_generator_should_work() {
    struct CustomAddressGenerator;

    impl AddressGenerator for CustomAddressGenerator {
        // deprecated from version 0.18.0
        fn next_address(&self, _storage: &mut dyn Storage) -> Addr {
            Addr::unchecked("deprecated")
        }

        // use this function instead of next_address
        fn contract_address(
            &self,
            _api: &dyn Api,
            _storage: &mut dyn Storage,
            _code_id: u64,
            _instance_id: u64,
        ) -> AnyResult<Addr> {
            Ok(Addr::unchecked("test_address"))
        }
    }

    // prepare wasm module with custom address generator
    let wasm_keeper = WasmKeeper::new().with_address_generator(CustomAddressGenerator);

    let mut app = AppBuilder::default().with_wasm(wasm_keeper).build(no_init);

    // store contract's code
    let code_id = app.store_code(test_contracts::counter::contract());

    let contract_addr = app
        .instantiate_contract(
            code_id,
            Addr::unchecked("owner"),
            &Empty {},
            &[],
            "Counter",
            None,
        )
        .unwrap();

    // make sure that contract address equals to "test_address" generated by custom address generator
    assert_eq!(contract_addr.as_str(), "test_address");
}

Anyway, let us append this test case to MultiTest after fixing, that also other developers can benefit from it.
Would you adjust your PR that I can just approve and merge it?
Thanks in advance!

Ahh, missed default function implementation in AddressGenerator trait. My bad, sorry!

Updated test accordingly. Also as next_address() becomes deprecated I think explicit panic in case it is called looks better in this test.

@epanchee epanchee marked this pull request as ready for review December 1, 2023 10:30
Copy link
Member

@DariuszDepta DariuszDepta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@DariuszDepta DariuszDepta merged commit 6887f7a into CosmWasm:main Dec 1, 2023
7 checks passed
@DariuszDepta DariuszDepta added this to the 0.20.0 milestone Dec 1, 2023
@epanchee epanchee deleted the test/broken_address_generator branch December 1, 2023 11:26
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

Successfully merging this pull request may close these issues.

2 participants