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.

Sign up for GitHub

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

Jump to bottom

Implement IBC-related APIs for Skip integration #4404

Merged
merged 15 commits into from
May 21, 2024
Merged

Implement IBC-related APIs for Skip integration #4404

merged 15 commits into from
May 21, 2024

Conversation

zbuc
Copy link
Member

@zbuc zbuc commented May 16, 2024 •
edited
Loading

Describe your changes

This changeset implements some APIs Skip indicated would be useful for their integration efforts.

Specifically:

  • cosmos.bank.v1beta1.Query/TotalSupply -- since we aren't using the Cosmos SDK, this didn't make sense to implement directly -- instead we've implemented a similar streaming penumbra.core.component.shielded_pool.v1.QueryService/TotalSupply call. Please note that only IBC transferred assets will have correct amounts set; internally tracked assets currently return a 0 amount (Skip said this is fine for their uses for now)
  • ibc.applications.transfer.v1.Query/DenomTraces
  • ibc.core.client.v1.Query/ClientStatus

I am not happy with some of the code I had to write to massage data into the right types but didn't have time to figure out anything better. I manually tested all three RPC methods locally and they seem to work as expected.

Issue ticket number and link

Closes #4391

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    Only creates new RPCs that return data from state

@conorsch conorsch mentioned this pull request May 21, 2024
Closed
@zbuc zbuc force-pushed the 4391_skip_ibc_apis branch from 1359441 to f848789 Compare May 21, 2024 15:28
@zbuc zbuc force-pushed the 4391_skip_ibc_apis branch from 0749232 to 6443c32 Compare May 21, 2024 20:47
@zbuc zbuc marked this pull request as ready for review May 21, 2024 20:47
@zbuc zbuc changed the title DRAFT: 4391 skip ibc apis Implement IBC-related APIs for Skip integration May 21, 2024
@zbuc zbuc force-pushed the 4391_skip_ibc_apis branch from 6443c32 to 8a06a7b Compare May 21, 2024 21:36
avahowell
avahowell approved these changes May 21, 2024
View reviewed changes
Copy link
Contributor

@avahowell avahowell left a comment

Choose a reason for hiding this comment

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

LGTM

crates/core/component/shielded-pool/src/component/rpc/transfer_query.rs
&self,
_: tonic::Request<QueryEscrowAddressRequest>,
) -> std::result::Result<tonic::Response<QueryEscrowAddressResponse>, tonic::Status> {
unimplemented!()
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting for the future that I think we can implement this by querying the value balance key, even though we don't have escrow accounts per se.

@conorsch
Copy link
Contributor

There's a CI failure here. On first pass, the check-all-features job failed, after which @zbuc added the "server" feature flag on the shielded-pool crate's ibc-proto import. However, that change broke the wasm-compat job. The former is preferable to the latter if we must break something, but let's try to get it passing before merge.

@conorsch
Copy link
Contributor

Nice, the latest feature-tweak resolved. Merging to get this included in upgrade testing, pending resolution of #4430.

@conorsch conorsch merged commit ea48cb4 into main May 21, 2024
13 checks passed
@conorsch conorsch deleted the 4391_skip_ibc_apis branch May 21, 2024 22:47
@hdevalence
Copy link
Member

image

We can't and shouldn't implement a custom RPC like this. The issue was to implement the RPC needed to interoperate with existing tooling, not to make up our own.

@zbuc zbuc mentioned this pull request May 23, 2024
Merged
1 task
conorsch pushed a commit that referenced this pull request May 24, 2024
@zbuc
Remove bespoke TotalSupply query, implement cosmos bank RPC instead (#…
478b1bf 
…4444)

This follows up on work in
#4404 by removing the
bespoke TotalSupply implementation and implementing the Cosmos Bank
`cosmos.bank.v1beta1.Query/TotalSupply` RPC instead.
@conorsch conorsch mentioned this pull request Jun 20, 2024
Merged
1 task
dynst added a commit to dynst/penumbra that referenced this pull request Jul 17, 2024
@dynst
shielded-pool: remove all use of unwrap
c34e704 
These were added in penumbra-zone#4404 and made `#![deny(clippy::unwrap_used)]` unhappy.

See also penumbra-zone#2920.
@dynst dynst mentioned this pull request Jul 17, 2024
Closed
1 task
dynst added a commit to dynst/penumbra that referenced this pull request Jul 21, 2024
@dynst
shielded-pool: remove all use of unwrap
8920993 
These were added in penumbra-zone#4404 and made `#![deny(clippy::unwrap_used)]` unhappy.

See also penumbra-zone#2920.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avahowell avahowell avahowell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Penumbra
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement missing IBC APIs for Skip
4 participants
@zbuc @conorsch @hdevalence @avahowell