Skip to content

IBC implementation review #234

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

Draft
wants to merge 38 commits into
base: main
Choose a base branch
from
Draft

IBC implementation review #234

wants to merge 38 commits into from

Conversation

DariuszDepta
Copy link
Member

No description provided.

@DariuszDepta DariuszDepta added this to the 3.0.0 milestone Jan 23, 2025
@DariuszDepta DariuszDepta self-assigned this Jan 23, 2025
@DariuszDepta DariuszDepta changed the title IBC implementation IBC implementation review Jan 23, 2025
@DariuszDepta DariuszDepta marked this pull request as draft January 23, 2025 12:53
Copy link
Contributor

@chipshort chipshort left a comment

Choose a reason for hiding this comment

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

Some comments about two of the issues. Have to look deeper into the other one.

self.process_ibc_response(api, contract_addr, storage, router, block, res)
}

fn ibc_packet_receive(
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to handle #197, you need to make sure that if contract.ibc_packet_receive returns an Err variant, this should not result in an error of this whole ibc_packet_receive function. It should instead return a json-encoded StdAck::Error(err_msg) where err_msg is the stringified error from the contract.
as documented in the CosmWasm IBC docs and implemented in Wasmd.

Comment on lines +339 to +340
// No acknowledgment needed
Ok(AppIbcReceiveResponse::default())
Copy link
Contributor

Choose a reason for hiding this comment

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

For #194, this should be the success ack (don't know about events or if we care to emulate the correct values there):

Suggested change
// No acknowledgment needed
Ok(AppIbcReceiveResponse::default())
Ok(AppIbcReceiveResponse {
events: vec![],
acknowledgement: Some(StdAck::success(b"\x01").to_binary()),
})

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