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 client for the client package. #378

Merged
merged 3 commits into from
Feb 8, 2023
Merged

Conversation

dzmitryhil
Copy link
Contributor

@dzmitryhil dzmitryhil commented Feb 6, 2023

This change is Reviewable

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @silverspase, @wojtek-coreum, and @ysv)


integration-tests/chain.go line 177 at r1 (raw file):

	// TODO(dhil) remove switch once crust is updated
	if strings.HasPrefix(cfg.GRPCAddress, "tcp") {

maybe instead of using tcp to differentiate the 2 different endpoints, we should use separate flags ?


integration-tests/init.go line 48 at r1 (raw file):

	)

	flag.StringVar(&coredAddress, "cored-address", "localhost:9090", "Address of cored node started by znet")

I think we can introduce rpc-address and grpc-address and then later remove this cored-address flag.

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)


integration-tests/chain.go line 177 at r1 (raw file):

Previously, miladz68 (milad) wrote…

maybe instead of using tcp to differentiate the 2 different endpoints, we should use separate flags ?

This is the temp solution, I'll remove it once we update the crust.


integration-tests/init.go line 48 at r1 (raw file):

Previously, miladz68 (milad) wrote…

I think we can introduce rpc-address and grpc-address and then later remove this cored-address flag.

Why do we need both for the tests? IMO one is enough.

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

a discussion (no related file):
Shouldn't we test both rpc and grpc endpoints?


Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

a discussion (no related file):

Previously, wojtek-coreum (Wojtek) wrote…

Shouldn't we test both rpc and grpc endpoints?

I've tested them locally by running the tests, what additionally do you propose?
I thought about the round-robin balancing for the transactions, but that might lead to not reproducible tests, which is very bad.


Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

a discussion (no related file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I've tested them locally by running the tests, what additionally do you propose?
I thought about the round-robin balancing for the transactions, but that might lead to not reproducible tests, which is very bad.

I'm just thinking about single test sending bank send over rpc. It would be also possible to run the full test suite, once using grpc and once using rpc but it would be an overkill IMO.


Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

a discussion (no related file):

Previously, wojtek-coreum (Wojtek) wrote…

I'm just thinking about single test sending bank send over rpc. It would be also possible to run the full test suite, once using grpc and once using rpc but it would be an overkill IMO.

anyway, later on we need to provide some protection against rpc issue you found


Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

a discussion (no related file):

Previously, wojtek-coreum (Wojtek) wrote…

anyway, later on we need to provide some protection against rpc issue you found

Right. But not for that PR.


wojtek-coreum
wojtek-coreum previously approved these changes Feb 8, 2023
Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @silverspase, and @ysv)

# Conflicts:
#	x/asset/nft/keeper/keeper.go
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @ysv)

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @ysv)

@dzmitryhil dzmitryhil merged commit f23252d into master Feb 8, 2023
@dzmitryhil dzmitryhil deleted the dzmitryhil/grpc-client branch February 8, 2023 13:07
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