Skip to content

Implement RPCs for client_02#196

Merged
majecty merged 3 commits intoCodeChain-io:ics-pocfrom
junha1:rpc
Feb 25, 2020
Merged

Implement RPCs for client_02#196
majecty merged 3 commits intoCodeChain-io:ics-pocfrom
junha1:rpc

Conversation

@junha1
Copy link
Contributor

@junha1 junha1 commented Feb 21, 2020

#Closes #192

@junha1
Copy link
Contributor Author

junha1 commented Feb 21, 2020

@majecty Please take a look at the interface. In short, compose_header() returns opaque(Bytes) data, whereas query_...() return transparent(JSON) data.

@majecty
Copy link

majecty commented Feb 21, 2020

@junha1 The interface looks good to me.

Now it uses same one with the Header.
@junha1 junha1 force-pushed the rpc branch 2 times, most recently from a6c83a0 to e464035 Compare February 24, 2020 08:38
@junha1 junha1 changed the title [WIP] Implement RPCs for client_02 Implement RPCs for client_02 Feb 24, 2020
@junha1 junha1 requested a review from majecty February 24, 2020 08:41

use crate::ibc::IdentifierSlice;

pub trait Nameable {
Copy link

Choose a reason for hiding this comment

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

The Nameable is used to print a debug message.
How about changing it like DebugName?
Then the name() should be changed to debug_name.

I found a discussion about trait name and it looks good to me.

If the trait has a single self-explanatory method (or a set of nearly identical methods), name it after the method: Clone, Hash, Default, Into, Write, ToOwned, AsRef, Extend.

rust-lang/api-guidelines#28 (comment)

#[serde(rename_all = "camelCase")]
pub struct IBCQuery {
pub number: u64,
pub data: String,
Copy link

Choose a reason for hiding this comment

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

Shouldn't we use the data as a generic parameter T?

Comment on lines +36 to +34
pub struct IBCQueryAbsence {
pub number: u64,
pub proof: Bytes,
}
Copy link

Choose a reason for hiding this comment

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

When we will use IBCQueryAbsence type?
IMO, we should make IBCQuery.data as optionable.

It provides interfaces for querying data / proof for ICS subdb.
@junha1 junha1 force-pushed the rpc branch 2 times, most recently from 1eb99a8 to 4420b69 Compare February 24, 2020 10:02
@junha1 junha1 requested a review from majecty February 24, 2020 10:03
@junha1
Copy link
Contributor Author

junha1 commented Feb 24, 2020

@majecty I updated all

Copy link

@majecty majecty left a comment

Choose a reason for hiding this comment

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

Please fix comments.

pub proof: Bytes,
}

/* -------- Client 02 -------- */
Copy link

Choose a reason for hiding this comment

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

We are not using /* comment.
Please use /// and use a full sentence.

#[serde(rename_all = "camelCase")]
pub struct ConsensusState {
pub validator_set_hash: H256,
// Unpacked CommitmentRoot
Copy link

Choose a reason for hiding this comment

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

Please us ///

}

/// Client 02 related types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an extra line to say that this comment is not only for the one right after (ClientState).

@majecty majecty merged commit 68862a4 into CodeChain-io:ics-poc Feb 25, 2020
@junha1 junha1 added experiment Experimental features ics Interchain standard labels Feb 27, 2020
@junha1 junha1 deleted the rpc branch May 25, 2020 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

experiment Experimental features ics Interchain standard

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants