cosmwasm-std instantiate2 implementation not accounting for chain contract address length

[gorgos]

The instantiate2_address helper inside cosmwasm-std is not accounting for the length of chain's contract addresses. Something like this would work:

fn instantiate2_address_impl(
    checksum: &[u8],
    creator: &CanonicalAddr,
    salt: &[u8],
    msg: &[u8],
) -> Result<CanonicalAddr, Instantiate2AddressError> {
   [...] (unchanged)

    let address_data = hash("module", &key);
    Ok(CanonicalAddr::from(&address_data[0..contractAddressLength]))
}

[webmaster128]

This is out of scope as Instantiate2 always uses the ADR for module accounts that always results in 32 byte addresses. All implementations of instantiate2 create 32 byte addresses.

[webmaster128]

Actually I wonder why we even have [:types.ContractAddrLen] in the wasmd code for Instantiate2. Looks like a bug to me. Can it be changed without forking wasmd? Seems to be a constant of 32.

[gorgos]

Yes it's done by forking wasmd, see e.g. https://github.com/InjectiveLabs/wasmd/blob/f/v0.40.2-inj-1/x/wasm/types/types.go#L21

[webmaster128]

Okay, then I think we should remove that part and always make it 32 byte addresses. The whole algorithm does not have a address length input. It's based on the "Basic Address" Hash from https://github.com/cosmos/cosmos-sdk/blob/v0.45.8/docs/architecture/adr-028-public-key-addresses.md which is all about moving from 20 byte to 32 byte addresses.

[gorgos]

We cannot use 32 byte addresses at Injective for various reasons and have also deemed the risks of having 20 byte addresses as acceptable. Other chains may choose non-32 byte addresses as well and instantiate2_address_impl could easily be modified to work correctly with different address lengths. So I don't see a reason to hard-code to 32 bytes. The 32 bytes could stay the default length obviously, but overriding should be possible.

[webmaster128]

I will ask around if we adapt the spec at some point in the future, but for now Instantate2 are fixed length 32 bytes outputs.

You can have a custom solution by just wrapping it in a custom manner like this:

pub fn my_address(
    checksum: &[u8],
    creator: &CanonicalAddr,
    salt: &[u8],
) -> Result<CanonicalAddr, Instantiate2AddressError> {
    Ok(instantiate2_address(checksum, creator, salt)?[..20].into())
}

[taitruong]

I will ask around if we adapt the spec at some point in the future, but for now Instantate2 are fixed length 32 bytes outputs.

You can have a custom solution by just wrapping it in a custom manner like this:

pub fn my_address(
    checksum: &[u8],
    creator: &CanonicalAddr,
    salt: &[u8],
) -> Result<CanonicalAddr, Instantiate2AddressError> {
    Ok(instantiate2_address(checksum, creator, salt)?[..20].into())
}

Ok, that's how to fix it for predicting instantiate2 address, but what about WasmMsg::Instantiate2? Does it also shorten and result to same address with length of 20?

[gorgos]

@taitruong The chain side is already truncating it accordingly: https://github.com/CosmWasm/wasmd/blob/5ec0c5ed3f93ab5eb8a3e1d3e923a6db28a27160/x/wasm/keeper/addresses.go#L71

[taitruong]

@taitruong The chain side is already truncating it accordingly: https://github.com/CosmWasm/wasmd/blob/5ec0c5ed3f93ab5eb8a3e1d3e923a6db28a27160/x/wasm/keeper/addresses.go#L71

Ty, ser! I've tested it on testnet for ics721 (public-awesome/cw-ics721#97) and it works like a charm!

[defser]

Any update on this? As @gorgos mentioned

@taitruong The chain side is already truncating it accordingly: https://github.com/CosmWasm/wasmd/blob/5ec0c5ed3f93ab5eb8a3e1d3e923a6db28a27160/x/wasm/keeper/addresses.go#L71

Ran into this issue today.
Any update on this? As @gorgos mentioned there is a difference between the result of the instantiate2_address and the chain side for chains where the length of chain's contract addresses is different.

[defser]

I will ask around if we adapt the spec at some point in the future, but for now Instantate2 are fixed length 32 bytes outputs.

You can have a custom solution by just wrapping it in a custom manner like this:

pub fn my_address(

    checksum: &[u8],

    creator: &CanonicalAddr,

    salt: &[u8],

) -> Result<CanonicalAddr, Instantiate2AddressError> {

    Ok(instantiate2_address(checksum, creator, salt)?[..20].into())

}

For now, I'll be using this solution, but it took me quite some time to figure out what went wrong and to find this post.

It should just work, and that will make future (injective) developers happy.

[chipshort]

This is on our radar and we will take a look at this for the 2.3 release. I think it would also make sense to make the wasmd const a var to allow chains to change it without having to fork.