Skip to content
This repository has been archived by the owner on Mar 5, 2021. It is now read-only.

Made other mod's public and removed missing_docs lint rule for now #6

Merged
merged 5 commits into from
Sep 15, 2020

Conversation

andynog
Copy link
Contributor

@andynog andynog commented Aug 27, 2020

Just some small modifications. Make other modes public other than just IBC. Also remove missing_docs lints for now until there's documentation and proto files are somewhat stable.

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGES.md

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

[I'm missing a bit of context here, so feel free to ignore my skepticism.]
Looks like major changes to various types (the prost Any special type and Height to name a couple), which we use in ibc-rs. Are we ready in ibc-rs to adapt to these changes?

Cargo.lock Outdated
@@ -88,7 +88,7 @@ dependencies = [

Copy link
Member

Choose a reason for hiding this comment

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

Should the .lock file be tracked in the repo?

Copy link
Member

Choose a reason for hiding this comment

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

It is recommended for projects which include a binary to track their Cargo.lock in version control: https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries

Copy link
Member

Choose a reason for hiding this comment

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

Though in this instance, since the main use case for this project is the library, we can actually omit it.

#[prost(uint64, tag="6")]
pub proof_height: u64,
#[prost(message, optional, tag="6")]
pub proof_height: ::std::option::Option<super::client::Height>,
Copy link
Member

Choose a reason for hiding this comment

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

The Height as defined here is a tuple of two u64, so it's important to note that this will likely impact our code (e.g., the TryFromRaw) and other parts.

Copy link
Member

Choose a reason for hiding this comment

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

let x = MsgCreateClient { signer: vec![] };
assert_eq!(x.signer, vec![]);
}
// Commenting this out. Until proto files are stable does not make sense to add unit tests
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should keep updating the generated .rs files if the source .proto files are unstable.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should pin against a specific version of the Cosmos SDK: https://github.com/informalsystems/ibc-proto/issues/10

@ancazamfir
Copy link

[I'm missing a bit of context here, so feel free to ignore my skepticism.]
Looks like major changes to various types (the prost Any special type and Height to name a couple), which we use in ibc-rs. Are we ready in ibc-rs to adapt to these changes?

Yes, I have started working on these changes. They are quite extensive but not much we can do, the sooner the better to understand them, make the changes and raise any concerns.

@romac romac self-requested a review September 15, 2020 13:59
romac
romac previously approved these changes Sep 15, 2020
@romac romac merged commit 73b87ce into master Sep 15, 2020
@romac romac deleted the andy/update-protos branch September 15, 2020 15:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants