Skip to content

Implement ConnOpenConfirm datagram #185

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 21, 2020
Merged

Implement ConnOpenConfirm datagram #185

merged 4 commits into from
Feb 21, 2020

Conversation

majecty
Copy link
Contributor

@majecty majecty commented Feb 18, 2020 •
edited
Loading

Depends on #180

@majecty majecty added the ics Interchain standard label Feb 18, 2020
@majecty majecty mentioned this pull request Feb 19, 2020
Closed
6 tasks
@majecty majecty force-pushed the f/ics-conn-open-confirm branch from 238a956 to 9355bd2 Compare February 20, 2020 09:40
@majecty majecty changed the title [WIP] Implementing ConnOpenConfirm datagram Implement ConnOpenConfirm datagram Feb 20, 2020
@majecty majecty force-pushed the f/ics-conn-open-confirm branch from 9355bd2 to e693dd5 Compare February 21, 2020 05:11
@majecty majecty requested a review from junha1 February 21, 2020 05:11
junha1
junha1 reviewed Feb 21, 2020
View reviewed changes
core/src/ibc/transaction_handler/datagrams.rs Outdated
},
ConnOpenConfirm {
identifier: String,
proof_ack: Vec<u8>,
Copy link
Contributor

Choose a reason for hiding this comment

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

How about Bytes for proofs and ctypes::BlockNumber for heights? I'll update previous ones (including this).

Copy link
Contributor Author

@majecty majecty Feb 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'll create a new commit that changes Vec to Bytes in all IBC code.
IMO, we should not use the ctypes::BlockNumber here. We should consider other chain's block number. ctypes::BlockNumber is for Foundry only.

Copy link
Contributor

@junha1 junha1 Feb 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ok then I'll leave Vec<u8>s. And please contain update to ctypes::BlockNumber for our blocks (e.g making proof), in the same commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about defining a IBCBlockNumber type instead?
I'm not sure which method is better, but using a IBCBlockNumber type in the all IBC code is easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe together with Identifier

@majecty majecty merged commit e79ab3e into CodeChain-io:ics-poc Feb 21, 2020
@majecty majecty deleted the f/ics-conn-open-confirm branch February 21, 2020 06:08
@junha1 junha1 added the experiment Experimental features label Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@junha1 junha1 junha1 approved these changes

Assignees

No one assigned

Labels

experiment Experimental features ics Interchain standard

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@majecty @junha1