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

[Service] Ran ignite scaffold type service --module service --yes #69

Merged
merged 7 commits into from
Oct 17, 2023

Conversation

Olshansk
Copy link
Member

@Olshansk Olshansk commented Oct 16, 2023

Summary

Created the service type by running the following command:

ignite scaffold type service --module service --yes

Human Summary

  • Added and defined the Service type plus supporting structures
  • Move the Service type from service to shared

AI Summary

Summary generated by Reviewpad on 17 Oct 23 14:56 UTC

This pull request introduces new protobuf files and modifies existing ones. It seems to resolve circular dependencies and add new definitions related to service configuration and identifiers. There are also some comments suggesting further research and future improvements.

Issue

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make test_all_unit
  • Verify Localnet manually: See the instructions [here](TODO: add link to instructions)

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have performed a self-review of my own code
  • I have commented my code, updated documentation and left TODOs throughout the codebase

@Olshansk Olshansk marked this pull request as draft October 16, 2023 17:44
@Olshansk Olshansk added the service Anything related to general-purpose RPC service support label Oct 16, 2023
@Olshansk Olshansk added this to the Shannon TestNet milestone Oct 16, 2023
@Olshansk Olshansk marked this pull request as ready for review October 16, 2023 17:50
@Olshansk Olshansk requested a review from red-0ne October 17, 2023 14:56
Copy link
Contributor

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Just a note regarding RPCType. In that setup we have no way to prevent a Supplier to declare an RPCType on its SupplierServiceConfig that does not exist for a given Service.
But this could also be the desired behavior if we assume that a Service should not enforce this (eg. Ethereum does not have an "official" GRPC RPCType but a Supplier could provide an implementation that does 🧠 )

@Olshansk Olshansk merged commit e4ac5e2 into main Oct 17, 2023
3 checks passed
@Olshansk Olshansk deleted the issues/10/service_type branch October 17, 2023 22:03
@Olshansk
Copy link
Member Author

@red-0ne That is indeed the desired behaviour!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service Anything related to general-purpose RPC service support
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants