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

feat: cw cluster connection #398

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

sherpalden
Copy link
Contributor

Description:

Commit Message

type: commit message

see the guidelines for commit messages.

Changelog Entry

version: <log entry>

Checklist:

  • I have performed a self-review of my own code
  • I have documented my code in accordance with the documentation guidelines
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have run the unit tests
  • I only have one commit (if not, squash them into one commit).
  • I have a descriptive commit message that adheres to the commit message guidelines

Please review the CONTRIBUTING.md file for detailed contributing guidelines.

@sherpalden sherpalden added the Cosmwasm Rust label Oct 21, 2024
@sherpalden sherpalden self-assigned this Oct 21, 2024
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 21, 2024
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.47%. Comparing base (231a239) to head (bdd5f01).
Report is 12 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #398   +/-   ##
=========================================
  Coverage     89.47%   89.47%           
  Complexity       77       77           
=========================================
  Files            42       42           
  Lines          2698     2698           
  Branches         37       37           
=========================================
  Hits           2414     2414           
  Misses          267      267           
  Partials         17       17           
Flag Coverage Δ
rust 90.29% <ø> (ø)
solidity 88.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

let validators_set = self.get_validators(deps.storage)?;

if threshold as usize > validators_set.len() {
return Err(ContractError::InvalidThreshold {
Copy link
Collaborator

Choose a reason for hiding this comment

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

length of validators should be greater than threshold. if threshold is greater than validator len it will never be approved.

src_network: NetId,
conn_sn: u128,
msg: String,
) -> Result<Response, ContractError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

where are signatures here? having this method will allow relayer to bypass signature check why are we having this? I saw this in another contract as well.

signatures,
)?;

if self.get_receipt(deps.as_ref().storage, src_network.clone(), conn_sn) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

first check receipt before validating signatures, so that we can return early.

}
}

pub fn reply(&self, deps: DepsMut, _env: Env, msg: Reply) -> Result<Response, ContractError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i dont think we would need to listen to replies in connection. if it fails on xcall everything will be reverted anyway and we are not doing anything on error reply as well.

msg: to_json_binary(&xcall_msg).unwrap(),
funds: vec![],
});
let sub_msg: SubMsg = SubMsg::reply_always(call_message, XCALL_HANDLE_MESSAGE_REPLY_ID);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can do reply never i think to make it synchronous and atomic.

funds: vec![],
});
let sub_msg: SubMsg = SubMsg::reply_always(call_message, XCALL_HANDLE_ERROR_REPLY_ID);
Ok(sub_msg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

deps.as_ref(),
threshold,
validators,
vec_msg.clone(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

the signed message should be srcNetwork+sn+msg not only message.

if signers.len() >= threshold.into() {
return Ok(());
}
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we doing break here ? wont it stop loop in first iteration?

}
}

Ok(validators_list)
Copy link
Collaborator

Choose a reason for hiding this comment

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

converting validators map to list and using that to lookup in signature verification? cant signature verification lookit up directly in the storage? if we dont want to touch storage we can return hashmap from here as well.

pub fn ripemd160(data: &[u8]) -> Vec<u8> {
use ripemd::Digest;
ripemd::Ripemd160::digest(&data).to_vec()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cosmwasm Rust documentation Improvements or additions to documentation feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants