-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add CW client extension traits #7
Add CW client extension traits #7
Conversation
…hub.com/informalsystems/cosmwasm-ibc into luca_joss/add-cw-client-extension-trait
This reverts commit 6489743.
self.deps_mut.as_mut() | ||
} | ||
|
||
fn generate_sha256_digest(&self, data: &[u8]) -> Checksum { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljoss17, by the way, is it necessary to be having this method here? Couldn't we just use Checksum::generate(data)
in the SN light client where this method is called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So Checksum
comes from cosmwasm_std
and would be required in the Wasm light-client, here https://github.com/informalsystems/ibc-starknet/blob/6c75fa50b233f973aaf3c138ae56832e4e3ab865/light-client/ibc-client-starknet/src/client_state/execution.rs#L65.
The issue is that the light-client
crate in ibc-starknet doesn't have the cosmwasm_std
dependency. So adding it would result in increasing the size of the contract and potentially the issue we already faced with the limited size allowed.
Does this seem correct to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now. My suggestion is:
Since this is being used temporarily for the 2nd demo there, it’s probably better to place this line of code in the ibc-client-starknet
crate for now and import it into the ibc-client-starknet-cw
. So, we can keep things clean here.
Regarding the size limit, I think we’re going to hit that limit eventually as we work towards completing the Starknet light client implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will close the PR in the meantime, and we can maybe keep the patch in ibc-starknet, https://github.com/informalsystems/ibc-starknet/blob/main/light-client/Cargo.toml#L64 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, let's go with a feature branch as Rano suggested and open ibc-starknet-related PRs against it. Once things consolidated, we can decide on whether to merge this feature branch into the main.
Can we switch the target branch to something called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. 🙏 Just resolve what Farhad suggested.
I also changed the base branch to starknet/demo2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
This PR adds
CwClientValidation
andCwClientExecution
traits allowing to retrieveDeps
andDepsMut
from `Context