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

two party POL holder #117

Merged
merged 49 commits into from
Nov 1, 2023
Merged

two party POL holder #117

merged 49 commits into from
Nov 1, 2023

Conversation

bekauz
Copy link
Collaborator

@bekauz bekauz commented Oct 9, 2023

closes #85 . blocked by #116 .

@bekauz bekauz requested a review from uditvira October 9, 2023 21:21
@bekauz bekauz mentioned this pull request Nov 1, 2023
@bekauz bekauz merged commit 3b041e8 into v2 Nov 1, 2023
Copy link
Collaborator

@Art3miX Art3miX left a comment

Choose a reason for hiding this comment

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

Very neat code! not much to say, just few little things.

Comment on lines +95 to +96
env.block.time,
env.contract.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.

Better just pass the &env directly instead of passing 2 values from it.

crate::msg::ExecuteMsg::Tick {},
)
.unwrap();
let mock_env = mock_env();
let msg_exp = CosmosMsg::Custom(NeutronMsg::IbcTransfer {
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 pick a better name, like expected_transfer_msg to make it clear what this msg is.

Comment on lines +63 to +66
// Verify caller is the clock
if info.sender != CLOCK_ADDRESS.load(deps.storage)? {
return Err(ContractError::Unauthorized {});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the helper here?

Comment on lines +53 to +65
for denom_split in &self.splits {
match &denom_split.split {
SplitType::Custom(config) => {
let remapped_split = config.remap_receivers_to_routers(
self.party_a_addr.to_string(),
party_a_router.to_string(),
self.party_b_addr.to_string(),
party_b_router.to_string(),
)?;
remapped_splits.push((denom_split.denom.to_string(), remapped_split));
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure whats going on here, but it looks weird.
I think a map will work better here.

}

impl SplitConfig {
pub fn remap_receivers_to_routers(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a redesign is needed here, it looks very complex and long for such a simple thing, might be missing some stuff.

Comment on lines +278 to +280
pub fn pass_blocks(&mut self, n: u64) {
self.app.update_block(|mut b| b.height += n);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

CW is so raw that its crazy!

Comment on lines +173 to +176
#[test]
fn test_holder_active_does_not_allow_claims() {
// unimplemented!()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can put the #[ignore] tag, even give a reason #[ignore = "reason"], it shows it when you run cargo test

Comment on lines +330 to +339
pub fn match_caller_party(&self, caller: String) -> Result<CovenantParty, StdError> {
let a = self.clone().party_a;
let b = self.clone().party_b;
if a.addr == caller {
Ok(a)
} else if b.addr == caller {
Ok(b)
} else {
Err(StdError::generic_err("unauthorized"))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be able to pass &caller to the function, and then avoid the clone and make it much simpler.

cargo test

lint:
cargo +nightly clippy --all-targets -- -D warnings && cargo +nightly fmt --all --check
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove +nightly here because we are not using it, and shouldn't use it actually (I think)

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 \
cosmwasm/workspace-optimizer-arm64:0.12.11
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sure to update optimize versions, its at 0.14 something i think.

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.

3 participants