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 (https://github.com/public-awesome/cw-ics721/pull/97) and it works like a charm!