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

Add newtypes for ids in ibc-union-spec #3410

Open
benluelo opened this issue Dec 29, 2024 · 1 comment
Open

Add newtypes for ids in ibc-union-spec #3410

benluelo opened this issue Dec 29, 2024 · 1 comment
Labels
A-cosmwasm Area: our CosmWasm stack A-ibc-union Area: ibc-union A-lightclients Area: Light Clients A-voyager Area: voyager (relayer) C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate.

Comments

@benluelo
Copy link
Contributor

Currently these are just raw type aliases:

type ClientId = u32;
type ConnectionId = u32;
type ChannelId = u32;

These should instead be proper newtypes wrapping a NonZeroU32 (since 0 is specified as the "nonexistentent" sentinel value for all ids). Anywhere where an id may be missing should be wrapped in an Option, similar to how it is done throughout the types used in ibc-classic-spec (example).

One issue that we'll hit (and the main reason why this isn't done yet) is that the state types in union-ibc require the keys and values to implement certain cosmwasm-specific traits in order to be used in the storage maps/items:

pub const QUERY_STORE: Item<Binary> = Item::new("query_store");
pub const CHANNEL_OWNER: Map<u32, Addr> = Map::new("channel_owner");
pub const CHANNELS: Map<u32, Channel> = Map::new("channels");
pub const CONTRACT_CHANNELS: Map<Addr, BTreeSet<u32>> = Map::new("contract_channels");
pub const CONNECTIONS: Map<u32, Connection> = Map::new("connections");
pub const CLIENT_STATES: Map<u32, Binary> = Map::new("client_states");
pub const CLIENT_CONSENSUS_STATES: Map<(u32, u64), Binary> = Map::new("client_consensus_states");
// From client type to contract implementation
pub const CLIENT_REGISTRY: Map<&str, Addr> = Map::new("client_registry");
// From client id to client type
pub const CLIENT_TYPES: Map<u32, String> = Map::new("client_types");
// From client id to contract implementation
pub const CLIENT_IMPLS: Map<u32, Addr> = Map::new("client_impls");
pub const NEXT_CLIENT_ID: Item<u32> = Item::new("next_client_id");
pub const NEXT_CONNECTION_ID: Item<u32> = Item::new("next_connection_id");
pub const NEXT_CHANNEL_ID: Item<u32> = Item::new("next_channel_id");

Rather than bringing in the cosmwasm traits into the ibc-union-spec crate (which we definitely don't want to do), we should instead keep the storage id key/values as raw u32s, and expose helpers that wrap in the relevant types. We already do this in some places:

fn next_channel_id(deps: DepsMut) -> Result<u32, ContractError> {
increment(deps, NEXT_CHANNEL_ID)
}

so hopefully this change won't be too drastic or invasive.

NOTE: This issue is labelled with all of A-cosmwasm, A-lightclients, and A-voyager since these spec crates touch a large portion of our codebase. That being said, the changes themselves are quite simple, just also quite pervasive. A small price to pay for type safety and reusable code 😁

@benluelo benluelo added C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. A-voyager Area: voyager (relayer) A-cosmwasm Area: our CosmWasm stack A-lightclients Area: Light Clients E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. and removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Dec 29, 2024
@benluelo benluelo added the A-ibc-union Area: ibc-union label Mar 4, 2025
@benluelo
Copy link
Contributor Author

benluelo commented Mar 4, 2025

This is soft blocked on #3919

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cosmwasm Area: our CosmWasm stack A-ibc-union Area: ibc-union A-lightclients Area: Light Clients A-voyager Area: voyager (relayer) C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate.
Projects
None yet
Development

No branches or pull requests

1 participant