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

instantiate2 & conditional ibc forwarder instantiation #141

Merged
merged 25 commits into from
Dec 31, 2023

Conversation

bekauz
Copy link
Collaborator

@bekauz bekauz commented Dec 15, 2023

Begins work towards #138 and introduces instantiate2. Feels like this is getting big enough to open the pr now.

  • use instantiate2 for covenant instantiation flow
  • only instantiate an ibc forwarder to facilitate party deposit if party is interchain. otherwise the deposit address is just the holder contract.
  • bump dependency versions in gh workflows and root level cargo
  • move v1 covenant components to a separate dir outside of workspace
  • no longer verifying clock whitelisted addresses to be contracts because of instantiate2
  • handling register_fee params for outgoing ibc messages from neutron (currently pulling from an open pr branch of neutron-sdk, need to revisit after all things are merged on their end)
  • interchain router now distributes fallback and target denoms separately

@bekauz bekauz added this to the v2 milestone Dec 15, 2023
@bekauz bekauz requested a review from Art3miX December 15, 2023 13:07
Comment on lines +88 to +102
pub fn to_instantiate2_msg(
&self,
admin_addr: String,
salt: Binary,
clock_address: String,
party_a_router: String,
party_b_router: String,
) -> Result<WasmMsg, StdError> {
let instantiate_msg =
match self.to_instantiate_msg(clock_address, party_a_router, party_b_router) {
Ok(msg) => msg,
Err(_) => {
return Err(StdError::generic_err(
"failed to generate regular instantiation message",
))
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not a contractError?

Comment on lines +128 to +137
let mut clock_whitelist = vec![
swap_holder_address.to_string(),
party_a_interchain_router_address.to_string(),
party_b_interchain_router_address.to_string(),
splitter_address.to_string(),
];
let preset_party_a_forwarder_fields = match msg.party_a_config.clone() {
CovenantPartyConfig::Interchain(config) => {
PARTY_A_IBC_FORWARDER_ADDR.save(deps.storage, &party_a_forwarder_address)?;
clock_whitelist.insert(0, party_a_forwarder_address.to_string());
Copy link
Collaborator

Choose a reason for hiding this comment

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

The size of the vector is known, I would crate a new vector with the needed capacity, and just push the needed info, I think it might be better.

contract: "splitter".to_string(),
err,
}),
let mut messages = Vec::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use with_capacity as the size is known

None => COVENANT_SWAP_HOLDER_ADDR.may_load(deps.storage)?,
}
} else {
Some(Addr::unchecked("not found"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change to None.

Comment on lines +43 to +60
fn get_precomputed_address(
deps: Deps,
code_id: u64,
creator: &CanonicalAddr,
salt: &[u8],
) -> Result<Addr, ContractError> {
let CodeInfoResponse { checksum, .. } = deps.querier.query_wasm_code_info(code_id)?;

let precomputed_address = instantiate2_address(&checksum, creator, salt)?;

Ok(deps.api.addr_humanize(&precomputed_address)?)
}

pub fn generate_contract_salt(salt_str: &[u8]) -> cosmwasm_std::Binary {
let mut hasher = Sha256::new();
hasher.update(salt_str);
hasher.finalize().to_vec().into()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those helper function should be in "covenant-package", doesn't make sense to copy them around from contract to contract, will be hell to make changes to that in the future.


workspace-optimize:
#!/bin/bash
docker run --rm -v "$(pwd)":/code \
--mount type=volume,source="$(basename "$(pwd)")_cache",target=/code/target \
--mount type=volume,source=registry_cache,target=/usr/local/cargo/registry \
--platform linux/amd64 \
cosmwasm/workspace-optimizer:0.12.13
cosmwasm/workspace-optimizer:0.14.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Already version 0.15 available :)

@@ -51,7 +51,7 @@ local-e2e-rebuild TEST: optimize
fi
cp -R artifacts/*.wasm {{TEST}}/tests/interchaintest/wasms
ls {{TEST}}/tests/interchaintest/wasms
cd {{TEST}}/tests/interchaintest/ && go test -timeout 50m -v
cd {{TEST}}/tests/interchaintest/ && go clean -testcache && go test -timeout 50m -v
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not add the testcache to here, this makes the test much slower, I would manually use this command when you know you need to clean cache.

@bekauz bekauz force-pushed the benskey/native-chain-routing branch from e943a22 to 14477b3 Compare December 31, 2023 12:33
@bekauz
Copy link
Collaborator Author

bekauz commented Dec 31, 2023

Great feedback. Will address it after merging.

@bekauz bekauz merged commit 58fcbb3 into v2 Dec 31, 2023
6 checks passed
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