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

Implement missing IBC APIs for Skip #4391

Closed
Tracked by #4402
zbuc opened this issue May 15, 2024 · 7 comments · Fixed by #4404
Closed
Tracked by #4402

Implement missing IBC APIs for Skip #4391

zbuc opened this issue May 15, 2024 · 7 comments · Fixed by #4404
Assignees
Labels
A-IBC Area: IBC integration with Penumbra C-enhancement Category: an enhancement to the codebase E-medium Effort: Medium E-week _P-high High priority _P-V1 Priority: slated for V1 release protobuf-changes Makes changes to the protobuf definitions.
Milestone

Comments

@zbuc
Copy link
Contributor

zbuc commented May 15, 2024

From Skip:

I took a look at which grpc endpoints are working that Skip API depends on, and most of the IBC ones seem to work. I found three that we depend on that would be benefitial if we could have them implemented.

cosmos.bank.v1beta1.Query/TotalSupply
https://github.com/cosmos/cosmos-sdk/blob/main/x/bank/proto/cosmos/bank/v1beta1/query.proto#L57

ibc.applications.transfer.v1.Query/DenomTraces
https://github.com/cosmos/ibc-go/blob/main/proto/ibc/applications/transfer/v1/query.proto#L16

ibc.core.client.v1.Query/ClientStatus
https://github.com/cosmos/ibc-go/blob/main/proto/ibc/core/client/v1/query.proto#L47
@zbuc zbuc added A-IBC Area: IBC integration with Penumbra C-enhancement Category: an enhancement to the codebase E-medium Effort: Medium E-week protobuf-changes Makes changes to the protobuf definitions. labels May 15, 2024
@github-project-automation github-project-automation bot moved this to Backlog in Penumbra May 15, 2024
@github-actions github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label May 15, 2024
@zbuc zbuc self-assigned this May 15, 2024
@zbuc zbuc moved this from Backlog to In progress in Penumbra May 15, 2024
@zbuc
Copy link
Contributor Author

zbuc commented May 15, 2024

@hdevalence Implementing the IBC APIs seems straightforward but cosmos.bank.v1beta1.Query/TotalSupply seems a little trickier as it's part of a larger CosmosSDK-specific RPC interface. Do we think it would be suitable to build a similar query within the existing shielded_pool API, or do we need to implement this specific call?

@aubrika aubrika added _P-V1 Priority: slated for V1 release _P-high High priority and removed needs-refinement unclear, incomplete, or stub issue that needs work labels May 15, 2024
@aubrika
Copy link
Contributor

aubrika commented May 15, 2024

@hdevalence Implementing the IBC APIs seems straightforward but cosmos.bank.v1beta1.Query/TotalSupply seems a little trickier as it's part of a larger CosmosSDK-specific RPC interface. Do we think it would be suitable to build a similar query within the existing shielded_pool API, or do we need to implement this specific call?

We should implement this specific call, but we may not include everything expected on other chains - this is for compatibility and may be idiosyncratic.

@conorsch
Copy link
Contributor

WIP PR here: #4404

@github-project-automation github-project-automation bot moved this from In progress to Done in Penumbra May 21, 2024
@hdevalence hdevalence reopened this May 23, 2024
@github-project-automation github-project-automation bot moved this from Done to In progress in Penumbra May 23, 2024
@hdevalence
Copy link
Member

This needs to implement the specified RPC: #4404 (comment)

Now that we merged a new RPC that we don't want we have to either deprecate it or do a breaking proto change.

@hdevalence
Copy link
Member

We need to immediately delete the following messages newly added to the Penumbra protobufs, before they are picked up and used:

rpc TotalSupply
TotalSupplyResponse
TotalSupplyRequest

The point of the issue was to implement the expected RPC, not to make up our own incompatible one.

@conorsch
Copy link
Contributor

We have a follow-up PR intended to close this out: #4444

@conorsch
Copy link
Contributor

Resolved via #4444

@github-project-automation github-project-automation bot moved this from In progress to Done in Penumbra May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-IBC Area: IBC integration with Penumbra C-enhancement Category: an enhancement to the codebase E-medium Effort: Medium E-week _P-high High priority _P-V1 Priority: slated for V1 release protobuf-changes Makes changes to the protobuf definitions.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants