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

[IBC] Clone cosmos/ics23 protobuf definitions into IBC repo #922

Merged
merged 7 commits into from
Jul 26, 2023

Conversation

h5law
Copy link
Contributor

@h5law h5law commented Jul 20, 2023

This PR copies in the cosmos/ics23 protobuf definition file into the IBC types directory.

Doing so allows the protoc compile to compile the protobuf definitions correctly such that they implement the proto.Message interface, as when compiled using the gogoproto fork used by cosmos they do not. This is to allow for the ProofSpec to be included in the light client PocketClientState message in #916. In this usecase the ics23.SmtSpec cannot be used directly due to the issues mention above.

Description

Summary generated by Reviewpad on 24 Jul 23 22:27 UTC

This pull request includes the following changes:

  • A newly added file containing the implementation of types and functions related to proofs in the ibc package. It includes conversion functions and a definition of a SmtSpec variable.
  • Changes in the "proofs.proto" file, including the addition of new enum types, message types, and options. These changes enhance the functionality related to proof generation and verification.
  • Changes in the Makefile, including the detection of the operating system, addition of OS-specific commands, a new target for downloading proto definitions, modification of imported package path, and a comment header indicating the origin of the proofs.proto file.

Please review the entire diff for further details.

Issue

Fixes N/A

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • Copy the cosmos/ics23 proto definition into the IBC types directory

Testing

  • make develop_test; if any code changes were made
  • make test_e2e on k8s LocalNet; if any code changes were made
  • e2e-devnet-test passes tests on DevNet; if any code was changed
  • Docker Compose LocalNet; if any major functionality was changed or introduced
  • k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@h5law h5law requested a review from Olshansk July 20, 2023 19:56
@reviewpad reviewpad bot added the large Pull request is large label Jul 20, 2023
@h5law h5law changed the base branch from main to persistence/update_block_header July 20, 2023 19:57
Base automatically changed from persistence/update_block_header to main July 20, 2023 21:47
@@ -0,0 +1,222 @@
syntax = "proto3";

// This file is a clone from the github.com/cosmos/ics23 repo, it has been
Copy link
Member

Choose a reason for hiding this comment

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

  1. I suggest even creating a small bash file that does a wget to the files you need (e.g. https://raw.githubusercontent.com/cosmos/ics23/master/proto/cosmos/ics23/v1/proofs.proto) so we can update it by running one command if it ever changes.

  2. Make sure to highlight this comment by starting with a // !! IMPORTANT !! This file is a clone

  3. What problems are you encountering or questions do you have? From my point of view, embedding the .proto definition instead of the compiled .pb.go file is the correct way to go about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if you look in #916 I need to embed the ProofSpec into the PocketClientState message. This is only possible when 1. we import the proto file and 2. the type is a message.

As the cosmos gogoproto compiler doesnt produce types that satisy the proto.Message interface we cannot just use the ics23.SmtSpec type they define we must recompile using normal protoc (so it produces proto.Message) types and then create the type ourselves. This way we can use the proto file, import it and then use the type. Adding the conversion allows us to still use the cosmos/ics23 library for the methods they define but gives us the ability to use the message aswell

@h5law h5law force-pushed the ibc/proto-exploration-ics23 branch from fbc8083 to 345f22d Compare July 21, 2023 09:49
@h5law h5law marked this pull request as ready for review July 21, 2023 09:49
@h5law h5law self-assigned this Jul 21, 2023
@h5law h5law added e2e-devnet-test Runs E2E tests on devnet ibc IBC specific changes and removed waiting-for-review labels Jul 21, 2023
@h5law h5law requested a review from Olshansk July 21, 2023 09:49
@h5law h5law force-pushed the ibc/proto-exploration-ics23 branch 3 times, most recently from f7a6dfe to 7116016 Compare July 21, 2023 09:59
@h5law h5law force-pushed the ibc/proto-exploration-ics23 branch from 56b12ac to 513005c Compare July 21, 2023 10:50
@Olshansk Olshansk added this to the M7: Pocket NoS (North Star) milestone Jul 24, 2023
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated
.PHONY: copy_ics23_proto
copy_ics23_proto:
echo "Downloading cosmos/ics23 proto definitions..."
curl -s -o ./ibc/types/proto/proofs.proto https://raw.githubusercontent.com/cosmos/ics23/master/proto/cosmos/ics23/v1/proofs.proto && \
Copy link
Member

Choose a reason for hiding this comment

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

I know both curl and wget get the job done, but I've seen wget used more frequently in these types of contexts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wget is not installed out of the box on most linux distros as such I think it better to use curl which is part of the coreutils.

Copy link
Member

Choose a reason for hiding this comment

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

Noted regarded coreutils. I have found wget (verified by chatgpt) to be the more common default, but not a block by any means.
Screenshot 2023-07-25 at 1 49 27 PM

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
ibc/types/proofs.go Show resolved Hide resolved
@h5law h5law requested a review from Olshansk July 24, 2023 22:27
@h5law h5law merged commit 5bfee56 into main Jul 26, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-devnet-test Runs E2E tests on devnet ibc IBC specific changes large Pull request is large waiting-for-review
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants