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

fix: Possibly helpful attempt at gogo #11662

Merged
merged 10 commits into from
May 9, 2022

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Apr 17, 2022

Description

Closes: #XXXX

Hi Robert, I'm sending this one because I think that it might be helpful. It may also be not useful much at all.

It is the result of my work to try and get Go workspaces going.

I figured that I might as well make this PR, since the gogo work really turned out to be very difficult and thought maybe this would be helpful. Right now the only clear issue is the type definitron for the Inteface Registry.

Cheers

-Jacob


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@nghuyenthevinh2000
Copy link

I have looked into the problem of Inteface Registry and has changed gateway JSONPb to use correct cosmos proto types at here:
https://github.com/notional-labs/gateway/tree/cosmos-gogo

@faddat
Copy link
Contributor Author

faddat commented Apr 19, 2022

Thanks Vinh! I'll see if this works better now-- it's key to getting workspaces for this repo

@faddat faddat changed the title Possibly helpful attempt at gogo fix: Possibly helpful attempt at gogo Apr 19, 2022
@faddat
Copy link
Contributor Author

faddat commented Apr 25, 2022

@nghuyenthevinh2000 fyi, I am going to merge @robert-zaremba's recent work into this, favoring his, and then I'm going to merge master in

I don't know if it is the right path, but I'd love to ship workspaces and build without containers (buf only) for 46.

Would be really useful for craft.

Ah, so, seems that Robert's branch has the same issue with the interface registry. I am going to merge yours into his. Can you please make a PR to the cosmos/gateway repo?

@nghuyenthevinh2000
Copy link

there is no such cosmos/gateway repo
Screenshot from 2022-04-25 18-54-24
for me

@faddat
Copy link
Contributor Author

faddat commented Apr 25, 2022

oops, wonder where I went wrong here

@faddat faddat marked this pull request as ready for review April 25, 2022 19:56
@faddat
Copy link
Contributor Author

faddat commented Apr 25, 2022

@nghuyenthevinh2000 I merged master here, to see which ci checks still fail

@faddat faddat marked this pull request as draft April 25, 2022 20:36
@faddat faddat mentioned this pull request Apr 25, 2022
19 tasks
cosmovisor/go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
orm/internal/listinternal/options.go Outdated Show resolved Hide resolved
server/config/config.go Show resolved Hide resolved
snapshots/chunk.go Outdated Show resolved Hide resolved
faddat and others added 2 commits April 27, 2022 19:01
Co-authored-by: Robert Zaremba <[email protected]>
Co-authored-by: Robert Zaremba <[email protected]>
faddat and others added 2 commits April 27, 2022 19:02
@faddat faddat marked this pull request as ready for review April 27, 2022 12:51
Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

I think we should keep "google.golang.org/protobuf/proto" and rollback that changes.

@@ -154,4 +155,6 @@ replace github.com/gin-gonic/gin => github.com/gin-gonic/gin v1.7.0

replace github.com/cosmos/cosmos-sdk/db => ./db

replace github.com/gogo/gateway => github.com/notional-labs/gateway v1.1.1-0.20220417180718-8e60e17a098d
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this replace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pbjson typing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry what I mean is:

this one was very important, it keeps the interface registry from exploding.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you mean? Can you link an issue? we specifically want to avoid replace.

orm/encoding/ormkv/entry.go Outdated Show resolved Hide resolved
@amaury1093 amaury1093 self-assigned this May 5, 2022
@robert-zaremba robert-zaremba merged commit bba18b1 into cosmos:robert/gogo May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants