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

gRPC blanket implementation from ibc-rs #125

Merged
merged 57 commits into from
Sep 20, 2023
Merged

gRPC blanket implementation from ibc-rs #125

merged 57 commits into from
Sep 20, 2023

Conversation

rnbguy
Copy link
Member

@rnbguy rnbguy commented Aug 24, 2023

This PR uses gRPC blanket implementation from ibc-rs using QueryContext and ProvableContext traits.

Todo:

  • Merge ibc-rs PR and update commit rev in this PR
  • Update Cargo.lock
  • Remove old service.rs
  • Add comments
  • Add more grpcurl tests
  • Fix client_status endpoint
  • Add unclog entry

@rnbguy rnbguy marked this pull request as ready for review September 7, 2023 13:54
@rnbguy rnbguy requested a review from plafer September 7, 2023 13:54
Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

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

A few nitpicks, then all good! tnx 🙏🏻

Cargo.toml Outdated Show resolved Hide resolved
crates/app/src/modules/ibc/impls.rs Outdated Show resolved Hide resolved
rnbguy and others added 4 commits September 15, 2023 12:26
Co-authored-by: Farhad Shabani <Farhad.Shabani@gmail.com>
Co-authored-by: Farhad Shabani <Farhad.Shabani@gmail.com>
@rnbguy
Copy link
Member Author

rnbguy commented Sep 15, 2023

I was thinking about how to make IbcContext's fields to have shared ownership across threads - since tonic services require it.

Currently, ibc_context.clone() will not create fields with shared ownership, as some fields are not wrapped in Arc<_>.

ExecutionContext updates these non-shared fields only in the main thread, which has the correct values of these fields. The results of ExecutionContext are written in Stores, whose ownership is shared with the tonic services. Now the tonic services only read data from these stores.

So even though the ibc_context in tonic services has incorrect values at non-shared-owned fields, the query responses are correct, as they don't read values from these fields.

So, the implementation of this PR works correctly, although the logic of the implementation is wrong.

Considering this, I was wondering if the fix should be in this PR or a separate one.

WDYT?

@Farhad-Shabani
Copy link
Member

Currently, ibc_context.clone() will not create fields with shared ownership, as some fields are not wrapped in Arc<_>.

Much needed improvement! This has caused me trouble in a few spots. Each time, I got caught up with other urgent tasks and didn't get around to update it.

Considering this, I was wondering if the fix should be in this PR or a separate one.
WDYT?

I'd suggest in a separate PR.

@rnbguy
Copy link
Member Author

rnbguy commented Sep 15, 2023

Looks like client_status calls host_consensus_state() which access one of those fields which are not wrapped in Arc<_> 😅 and it threw error as expected when that endpoint is queried.

I will wrap only that field to keep this PR wokring. The Arc<_> wrap for other fields will be on future PR.

@plafer plafer merged commit 63abb18 into main Sep 20, 2023
10 checks passed
@plafer plafer deleted the rano/ibc-grpc-traits branch September 20, 2023 19:59
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.

3 participants