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

Replace the Bytes alias in CosmWasm context with cosmwasm_std::Binary #1271

Closed
srdtrk opened this issue Jul 8, 2024 · 4 comments · Fixed by #1272
Closed

Replace the Bytes alias in CosmWasm context with cosmwasm_std::Binary #1271

srdtrk opened this issue Jul 8, 2024 · 4 comments · Fixed by #1272
Assignees
Labels
A: breaking Admin: breaking change that may impact operators O: maintainability Objective: cause to ease modification, fault corrections and improve code understanding
Milestone

Comments

@srdtrk
Copy link
Member

srdtrk commented Jul 8, 2024

Improvement Summary

Currently, the CosmWasm context uses a Bytes alias to serialize/deserialize Base64 strings to bytes through the use of Serde tags (for example, see here). However, Bytes can be replaced with cosmwasm_std::Binary, which allows us to remove all the Serde and Schemars tags. Here is an example of the proposed change:

#[cw_serde]
pub struct InstantiateMsg {
-    #[schemars(with = "String")]
-    #[serde(with = "Base64", default)]
-    pub client_state: Bytes,
-    #[schemars(with = "String")]
-    #[serde(with = "Base64", default)]
-    pub consensus_state: Bytes,
-    #[schemars(with = "String")]
-    #[serde(with = "Base64", default)]
-    pub checksum: Bytes,
+    pub client_state: cosmwasm_std::Binary,
+    pub consensus_state: cosmwasm_std::Binary,
+    pub checksum: cosmwasm_std::Binary,
}

Context

As part of the ongoing migration in the 08-wasm contract API, we recommend moving away from using the Bytes alias in favor of cosmwasm_std::Binary.

The recent changes to the merkle path in 08-wasm can be seen here. Until more generic paths are supported in ibc-rs (see issue #1255), these bytes must be asserted as UTF-8 strings before being passed down to the Tendermint context.

By making this change, we simplify the code and reduce potential points of failure related to serialization/deserialization.

@Farhad-Shabani Farhad-Shabani self-assigned this Jul 8, 2024
@Farhad-Shabani Farhad-Shabani added the O: maintainability Objective: cause to ease modification, fault corrections and improve code understanding label Jul 8, 2024
@github-project-automation github-project-automation bot moved this to 📥 To Do in ibc-rs Jul 8, 2024
@Farhad-Shabani Farhad-Shabani added this to the 0.54.0 milestone Jul 8, 2024
@Farhad-Shabani Farhad-Shabani added the A: breaking Admin: breaking change that may impact operators label Jul 8, 2024
@Farhad-Shabani
Copy link
Member

This is a pleasant improvement ✨. Thank you @srdtrk for the context.

I have opened up PR #1272. Could you check if it works?

By the way, couldn't quite understand how the MerklePath changes are relevant here. I see we are moving from strings to bytes there, but we have been working with bytes here anyway (either Bytes or Binary). Am I missing something?

@Farhad-Shabani
Copy link
Member

Just note that: upon the changes in #1272, I realized to consistently use cosmwasm_std::Binary everywhere in ibc-rs instead of Bytes, Binary needs to support no_std as well, which it does not at the moment. Therefore, we can only apply this update on the ibc-client-cw crate, and ibc-client-wasm-types will remain as is.

@srdtrk
Copy link
Member Author

srdtrk commented Jul 9, 2024

Yes, sorry for my ambiguous language. MerklePath changes are not directly related. I was trying to suggest that since we are moving from strings to bytes in MerklePath, we should use a Vec<Binary> instead of Vec<String> here once #1255 is resolved.

Even if #1255 is not resolved, we should still switch to Vec<Binary> asap and then cast it to a string here. Do you think a PR can be opened for this?

@Farhad-Shabani
Copy link
Member

Got it. Thanks. To keep things consistent throughout ibc-rs, we most likely need to come up with our own domain MerklePath definition. I’m working on a solution to address #1255 and incorporate bytes into the MerklePath at the same time. Here’s the draft #1273 if you want to take a look. Let’s see where we end up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: breaking Admin: breaking change that may impact operators O: maintainability Objective: cause to ease modification, fault corrections and improve code understanding
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants