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

feat(grpc, types, proto)!: Add tonic gRPC #454

Open
wants to merge 57 commits into
base: main
Choose a base branch
from
Open

feat(grpc, types, proto)!: Add tonic gRPC #454

wants to merge 57 commits into from

Conversation

fl0rek
Copy link
Member

@fl0rek fl0rek commented Oct 24, 2024

No description provided.

fl0rek
fl0rek commented Nov 4, 2024
View reviewed changes
rpc/src/lib.rs Outdated Show resolved Hide resolved
@fl0rek fl0rek marked this pull request as ready for review November 4, 2024 09:50
zvolin
zvolin reviewed Nov 4, 2024
View reviewed changes
proto/build.rs Outdated Show resolved Hide resolved
proto/vendor/celestia/da/data_availability_header.pb.go Outdated Show resolved Hide resolved
zvolin
zvolin reviewed Nov 5, 2024
View reviewed changes
proto/Cargo.toml Outdated Show resolved Hide resolved
zvolin
zvolin reviewed Nov 5, 2024
View reviewed changes
proto/src/lib.rs Outdated Show resolved Hide resolved
zvolin
zvolin reviewed Nov 5, 2024
View reviewed changes
rpc/Cargo.toml Outdated Show resolved Hide resolved
zvolin
zvolin reviewed Nov 5, 2024
View reviewed changes
tonic/Cargo.toml Outdated Show resolved Hide resolved
zvolin
zvolin reviewed Nov 5, 2024
View reviewed changes
tonic/src/tendermint.rs Outdated Show resolved Hide resolved
zvolin
zvolin reviewed Nov 5, 2024
View reviewed changes
tonic/src/types.rs Outdated Show resolved Hide resolved
zvolin
zvolin reviewed Nov 5, 2024
View reviewed changes
tonic/src/client.rs Outdated Show resolved Hide resolved
zvolin
zvolin reviewed Nov 5, 2024
View reviewed changes
types/Cargo.toml Outdated Show resolved Hide resolved
zvolin
zvolin reviewed Nov 5, 2024
View reviewed changes
tools/update-proto-vendor.sh Outdated Show resolved Hide resolved
@fl0rek fl0rek force-pushed the feat/grpc branch 2 times, most recently from 4d146d9 to b9a99f0 Compare November 21, 2024 15:10
zvolin
zvolin reviewed Nov 21, 2024
View reviewed changes
ci/run-validator.sh Outdated Show resolved Hide resolved
zvolin
zvolin reviewed Nov 21, 2024
View reviewed changes
grpc/Cargo.toml Outdated Show resolved Hide resolved
fl0rek and others added 2 commits November 21, 2024 17:08
@fl0rek @zvolin
Update grpc/Cargo.toml
3c1d7c1 
Co-authored-by: Maciej Zwoliński <[email protected]>
Signed-off-by: Mikołaj Florkiewicz <[email protected]>
@fl0rek
cleanup
9a835ed
oblique
oblique reviewed Nov 22, 2024
View reviewed changes
Cargo.toml Outdated
@@ -1,20 +1,23 @@
[workspace]
resolver = "2"
members = ["cli", "node", "node-wasm", "proto", "rpc", "types"]
members = ["cli", "node", "node-wasm", "proto", "rpc", "grpc", "types"]
Copy link
Member

Choose a reason for hiding this comment

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

I think grpc/grpc-macros should be specified too.

grpc/src/types/auth.rs Outdated

/// Enum representing different types of account
#[derive(Debug, PartialEq)]
#[non_exhaustive]
Copy link
Member

Choose a reason for hiding this comment

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

Why non_exhaustive?

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking was to be future compatible in case new account type pops up. Upon retrospection I'm not sure that's needed and we can just release breaking version in that case.

proto/src/lib.rs Outdated Show resolved Hide resolved
zvolin
zvolin reviewed Nov 22, 2024
View reviewed changes
proto/build.rs Outdated Show resolved Hide resolved
zvolin
zvolin reviewed Nov 22, 2024
View reviewed changes
types/src/auth.rs Outdated Show resolved Hide resolved
zvolin
zvolin reviewed Nov 22, 2024
View reviewed changes
types/src/bit_array.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

could we move this and auth to the state/?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, moved 👍

Copy link
Member

Choose a reason for hiding this comment

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

I think you forgot to move bit_array

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought of it as a container type, which is why I put it in root, but actually we can move it to state since it's only used there.

zvolin
zvolin reviewed Nov 22, 2024
View reviewed changes
Copy link
Member

@zvolin zvolin left a comment

Choose a reason for hiding this comment

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

left some last comments, otherwise lgtm

zvolin
zvolin approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@zvolin zvolin left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oblique oblique oblique left review comments

@zvolin zvolin zvolin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fl0rek @oblique @zvolin