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

feat: params query #433

Merged
merged 3 commits into from
Dec 9, 2024
Merged

feat: params query #433

merged 3 commits into from
Dec 9, 2024

Conversation

gluax
Copy link
Contributor

@gluax gluax commented Dec 7, 2024

Motivation

Seems that most built in modules support querying the params from GRPC/CLI.

This would make my life much easier on the test suite side as well.

Explanation of Changes

I added a new Params query to the pubkey module, matching how other sdk modules add the params query.
I also added the CLI/Go code to support the above.

As for the other protobuf changes, I'm not sure why the make proto-all command was removing the 1 line from a few files.

Currently branch is based on main, if it's a pain I can re-base and target #431 instead @hacheigriega?

Testing

Tested with test suite.

image

Should I somehow add other tests?

Related PRs and Issues

N/A

@gluax gluax self-assigned this Dec 7, 2024
Copy link
Member

@Thomasvdam Thomasvdam left a comment

Choose a reason for hiding this comment

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

As for the other protobuf changes, I'm not sure why the make proto-all command was removing the 1 line from a few files.

Last time we had this is was because we were using different versions of the proto toolchain. I'm using buf 1.47.2

proto/sedachain/pubkey/v1/query.proto Outdated Show resolved Hide resolved
@gluax
Copy link
Contributor Author

gluax commented Dec 9, 2024

As for the other protobuf changes, I'm not sure why the make proto-all command was removing the 1 line from a few files.

Last time we had this is was because we were using different versions of the proto toolchain. I'm using buf 1.47.2

@Thomasvdam I updated my buf version to match yours, but it seems those 1 line changes remain the same.

@Thomasvdam
Copy link
Member

As for the other protobuf changes, I'm not sure why the make proto-all command was removing the 1 line from a few files.

Last time we had this is was because we were using different versions of the proto toolchain. I'm using buf 1.47.2

@Thomasvdam I updated my buf version to match yours, but it seems those 1 line changes remain the same.

That's odd, when I run it on your branch it adds all the lines back. Are you sure the shell session executing the proto-gen has the correct buf version in its path?

@gluax
Copy link
Contributor Author

gluax commented Dec 9, 2024

As for the other protobuf changes, I'm not sure why the make proto-all command was removing the 1 line from a few files.

Last time we had this is was because we were using different versions of the proto toolchain. I'm using buf 1.47.2

@Thomasvdam I updated my buf version to match yours, but it seems those 1 line changes remain the same.

That's odd, when I run it on your branch it adds all the lines back. Are you sure the shell session executing the proto-gen has the correct buf version in its path?

image

I'm using the make proto-all command to generate the protobuf, is that different from what you're using?

@gluax gluax force-pushed the feat/params branch 2 times, most recently from 8551abf to 5e7e9f8 Compare December 9, 2024 14:58
@gluax
Copy link
Contributor Author

gluax commented Dec 9, 2024

@Thomasvdam and I learned I had to do make proto-dep-install first.

@gluax gluax requested a review from Thomasvdam December 9, 2024 15:14
Copy link
Member

@Thomasvdam Thomasvdam left a comment

Choose a reason for hiding this comment

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

Whoops

@gluax gluax merged commit 82bd3a3 into main Dec 9, 2024
16 of 17 checks passed
@gluax gluax deleted the feat/params branch December 9, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants