-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
chore: remove sdk imports from server v2 #19744
Conversation
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
gentle ping on this, can we get this wrapped up. i can pick it up if needed |
@tac0turtle whoops, dropped the ball on this one. Will get back to it tomorrow 👌 |
|
||
// DefaultPage is the default `page` number for queries. | ||
// If the `page` number is not supplied, `DefaultPage` will be used. | ||
const DefaultPage = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pagination seems like a generic thing, any reason we cant put it into the api pkg at the in server/v2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we put a variation of it in core. This is to avoid sdk imports of pagination in modules. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also a proto generated file that contains PageRequest and some other. I think this is also extracted from comet, so if we want to extend this same pagination style everywhere else then we should make it like you said
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use the api module here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we talked about this on a call, but that would mean we use a pulsar type. Also, those types use codec/any for pubkeys, so we are still going to have a dependency on the sdk EDIT: I'll double-check this last part, I think I got them mixed up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so i think this is fine for now, we should get this merged, but we will need to do another refactor here because we dont want to use the pulsar types. Im not sure where pagination should live to avoid the imports, maybe you can dive into this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused, should we not use pulsar on this pr? I started without it but then we talked about using it so I started to use it and added a bunch of utils to make that possible here: use cosmossdk.io/api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be able to depend on server v2 here, we can put it in there. the issue is we are adding multiple versions of pagination now, which makes client world harder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is still work to do there.
@@ -0,0 +1,232 @@ | |||
package genutil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where does this get used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used in getGenDocProvider
when creating the node. I put it in a different package to make it easier to remove later if needed
|
||
// DefaultPage is the default `page` number for queries. | ||
// If the `page` number is not supplied, `DefaultPage` will be used. | ||
const DefaultPage = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so i think this is fine for now, we should get this merged, but we will need to do another refactor here because we dont want to use the pulsar types. Im not sure where pagination should live to avoid the imports, maybe you can dive into this?
int64 voting_power = 3; | ||
int64 proposer_priority = 4; | ||
string address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | ||
.tendermint.crypto.PublicKey pub_key = 2 [(gogoproto.nullable) = false]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because google.protobuf.Any
will use the sdk's codec package, which is unnecessary here if we use the original types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah true, dam that type. It was moved now but i guess the code gen needs to be updated
|
||
option go_package = "github.com/cosmos/cosmos-sdk/client/grpc/cmtservice"; | ||
option go_package = "github.com/cosmos/cosmos-sdk/server/v2/cometbft/client/grpc/cmtservice"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens to clients version? part of me thinks this needs to redefined
Description
Stuff missing/to revisit:
Copy pagination.proto to cometbft types, fix proto generation to generate on the correct directoryFigure out types.Any and cryptotypes.PubKeyAlsogenutiltypes.AppGenesisFromFile
+ sdk versionFollow up PR needed: to add the app version info to the GetNodeInfo method
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...
!
in the type prefix if API or client breaking changeCHANGELOG.md
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...