Encoding Server, Go Request builder package, client example #262
Conversation
WalkthroughThe changes encompass various updates and additions across multiple files, focusing on enhanced functionalities related to gRPC interactions, encoding services, and client-side operations. Key enhancements include new Go modules for building gRPC schedule requests, TLS authentication, encoding services, and better integration testing setups alongside expanded documentation and improved proto definitions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Application
participant GRPC Server as gRPC Server
participant TLS as TLS Service
participant Encoding as Encoding Service
participant Builder as Builder Service
Client->>TLS: buildCredentials()
TLS-->>Client: TransportCredentials
Client->>GRPC Server: getConnection(TransportCredentials)
GRPC Server-->>Client: ClientConn
Client->>Builder: buildScheduleRequest()
Builder-->>Client: ScheduleRequest
Client->>GRPC Server: SendRequest(ScheduleRequest)
GRPC Server->>Encoding: EncodeRequest(CallData)
Encoding-->>GRPC Server: EncodeResponse(EncodedCall)
GRPC Server-->>Client: Response(EncodedCall)
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (1)
README.md (1)
Line range hint
32-32: Grammar adjustment for collective noun agreement.The phrase "a consensus of validators submit" should be corrected to "a consensus of validators submits" for grammatical accuracy.
- ...r contract if a consensus of validators submit the same cork. + ...r contract if a consensus of validators submits the same cork.Tools
Markdownlint
13-13: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
30-30: null (MD019, no-multiple-space-atx)
Multiple spaces after hash on atx style heading
26-26: null (MD034, no-bare-urls)
Bare URL used
README.md
Outdated
|
|
||
| Or download the latest release here: | ||
|
|
||
| https://github.com/PeggyJV/steward/releases/tag/v4.2.3 |
There was a problem hiding this comment.
Provide a descriptive hyperlink for the release download.
While the link to the latest release is helpful, it would be more user-friendly to use a descriptive hyperlink instead of a bare URL.
- https://github.com/PeggyJV/steward/releases/tag/v4.2.3
+ [Download the latest release here](https://github.com/PeggyJV/steward/releases/tag/v4.2.3)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| https://github.com/PeggyJV/steward/releases/tag/v4.2.3 | |
| [Download the latest release here](https://github.com/PeggyJV/steward/releases/tag/v4.2.3) |
Tools
Markdownlint
26-26: null (MD034, no-bare-urls)
Bare URL used
src/server.rs
Outdated
| let address = socket_addr(config)?; | ||
|
|
||
| info!("encoding mode disabled"); |
There was a problem hiding this comment.
Clarify the purpose of the log message.
The log message "encoding mode disabled" in the load_server_config function could be misleading as it suggests that the encoding mode is always disabled, which contradicts the addition of encoding functionality. Consider revising this message for clarity.
- info!("encoding mode disabled");
+ info!("loading standard server configuration");Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let address = socket_addr(config)?; | |
| info!("encoding mode disabled"); | |
| let address = socket_addr(config)?; | |
| info!("loading standard server configuration"); |
client_example/main.go
Outdated
| func main() { | ||
| // Replace these paths with your own client cert and key, and server CA paths | ||
| creds, err := buildCredentials("client.crt", "client.key", "server_ca.crt") | ||
| if err != nil { | ||
| fmt.Printf("failed to build credentials: %v", err) | ||
| return | ||
| } | ||
|
|
||
| cellarId := common.HexToAddress("") | ||
| chainId := uint(1) | ||
| adaptorContract := common.HexToAddress("") | ||
| blockHeight := uint(1) | ||
|
|
||
| request := BuildUniswapV3ScheduleRequest(cellarId, chainId, adaptorContract, blockHeight) | ||
|
|
||
| addr := fmt.Sprintf("localhost:5734") | ||
| conn, err := getConnection(addr, creds) | ||
| if err != nil { | ||
| fmt.Printf("failed to get connection: %v", err) | ||
| return | ||
| } | ||
| defer conn.Close() | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
|
|
||
| c := steward_proto.NewContractCallServiceClient(conn) | ||
| _, err = c.Schedule(ctx, request) | ||
| } |
There was a problem hiding this comment.
Potential security issue with hardcoded credential paths.
The paths for client certificates and keys are hardcoded. This could lead to security vulnerabilities if the code is used in a production environment without modification.
- creds, err := buildCredentials("client.crt", "client.key", "server_ca.crt")
+ // TODO: Replace hardcoded paths with configurable pathsCommittable suggestion was skipped due to low confidence.
client_example/main.go
Outdated
| func buildCredentials(clientCertPath string, clientKeyPath string, serverCAPath string) (credentials.TransportCredentials, error) { | ||
| clientCert, err := tls.LoadX509KeyPair(clientCertPath, clientKeyPath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| rootPool := x509.NewCertPool() | ||
| serverCA, err := os.ReadFile(serverCAPath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| rootPool.AppendCertsFromPEM(serverCA) | ||
|
|
||
| tlsConfig := &tls.Config{ | ||
| Certificates: []tls.Certificate{clientCert}, | ||
| RootCAs: rootPool, | ||
| } | ||
|
|
||
| return credentials.NewTLS(tlsConfig), nil |
There was a problem hiding this comment.
Improve error handling and resource management in TLS configuration.
Consider handling potential errors from AppendCertsFromPEM and ensuring that all resources are cleaned up properly in case of errors.
+ if !rootPool.AppendCertsFromPEM(serverCA) {
+ return nil, fmt.Errorf("failed to append server CA")
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func buildCredentials(clientCertPath string, clientKeyPath string, serverCAPath string) (credentials.TransportCredentials, error) { | |
| clientCert, err := tls.LoadX509KeyPair(clientCertPath, clientKeyPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| rootPool := x509.NewCertPool() | |
| serverCA, err := os.ReadFile(serverCAPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| rootPool.AppendCertsFromPEM(serverCA) | |
| tlsConfig := &tls.Config{ | |
| Certificates: []tls.Certificate{clientCert}, | |
| RootCAs: rootPool, | |
| } | |
| return credentials.NewTLS(tlsConfig), nil | |
| func buildCredentials(clientCertPath string, clientKeyPath string, serverCAPath string) (credentials.TransportCredentials, error) { | |
| clientCert, err := tls.LoadX509KeyPair(clientCertPath, clientKeyPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| rootPool := x509.NewCertPool() | |
| serverCA, err := os.ReadFile(serverCAPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if !rootPool.AppendCertsFromPEM(serverCA) { | |
| return nil, fmt.Errorf("failed to append server CA") | |
| } | |
| tlsConfig := &tls.Config{ | |
| Certificates: []tls.Certificate{clientCert}, | |
| RootCAs: rootPool, | |
| } | |
| return credentials.NewTLS(tlsConfig), nil | |
| } |
client_example/schedule_request.go
Outdated
| func BuildUniswapV3ScheduleRequest(cellarId common.Address, chainId uint, adaptorContract common.Address, blockHeight uint) *steward_proto.ScheduleRequest { | ||
| return &steward_proto.ScheduleRequest{ | ||
| CellarId: cellarId.Hex(), | ||
| ChainId: uint64(chainId), | ||
| CallData: &steward_proto.ScheduleRequest_CellarV2_5{ | ||
| CellarV2_5: &steward_proto.CellarV2_5{ | ||
| CallType: &steward_proto.CellarV2_5_FunctionCall_{ | ||
| FunctionCall: &steward_proto.CellarV2_5_FunctionCall{ | ||
| Function: &steward_proto.CellarV2_5_FunctionCall_CallOnAdaptor{ | ||
| CallOnAdaptor: &steward_proto.CellarV2_5_CallOnAdaptor{ | ||
| Data: []*steward_proto.AdaptorCall{ | ||
| SwapWithUniswapV3AdaptorCall( | ||
| adaptorContract, | ||
| []string{ | ||
| "0x00000000000000000000000000000000000000000000", | ||
| "0x11111111111111111111111111111111111111111111", | ||
| "0x22222222222222222222222222222222222222222222", | ||
| }, | ||
| []uint32{1000, 2000, 3000}, | ||
| "100000000", | ||
| "100000000", | ||
| ), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| BlockHeight: uint64(blockHeight), | ||
| Deadline: 0, | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimize the structure initialization in BuildUniswapV3ScheduleRequest.
The function correctly builds a schedule request but consider using a constructor or a builder pattern to manage complex object creation more effectively.
+ // Consider using a builder pattern to simplify and manage complex object creationCommittable suggestion was skipped due to low confidence.
| #[async_trait] | ||
| impl proto::encoding_service_server::EncodingService for EncodingHandler { | ||
| async fn encode( | ||
| &self, | ||
| request: Request<EncodeRequest>, | ||
| ) -> Result<Response<EncodeResponse>, Status> { | ||
| let request = request.get_ref().to_owned(); | ||
| let _ = match &request.cellar_id.parse::<Address>() { | ||
| Ok(a) => a, | ||
| Err(_) => { | ||
| return Err(Status::new( | ||
| Code::InvalidArgument, | ||
| format!("invalid EVM address format {:?}", request.cellar_id), | ||
| )); | ||
| } | ||
| }; | ||
| let encoded_call = match get_encoded_call(request) { | ||
| Ok(c) => c, | ||
| Err(err) => { | ||
| warn!("failed to get encoded call with error {:?}", err); | ||
| return Err(Status::new(Code::InvalidArgument, err.to_string())); | ||
| } | ||
| }; | ||
|
|
||
| Ok(Response::new(EncodeResponse { encoded_call })) | ||
| } |
There was a problem hiding this comment.
Review the implementation of EncodingHandler.
Ensure that the error handling is robust and that the encoding logic correctly handles different types of calls. Consider adding more detailed logging for debugging purposes.
+ // Add more detailed logging hereCommittable suggestion was skipped due to low confidence.
| fn get_encoded_call(request: EncodeRequest) -> Result<String, Box<dyn std::error::Error>> { | ||
| if request.call_data.is_none() { | ||
| return Err(ErrorKind::Http.context("empty call data").into()); | ||
| } | ||
|
|
||
| let call_data = match request.call_data.unwrap() { | ||
| AaveV2Stablecoin(call) => { | ||
| if call.function.is_none() { | ||
| return Err(ErrorKind::Http.context("empty function data").into()); | ||
| } | ||
|
|
||
| aave_v2_stablecoin::get_encoded_call(call.function.unwrap(), request.cellar_id) | ||
| } | ||
| CellarV1(call) => { | ||
| if call.function.is_none() { | ||
| return Err(ErrorKind::Http.context("empty function data").into()); | ||
| } | ||
|
|
||
| cellar_v1::get_encoded_call(call.function.unwrap(), request.cellar_id) | ||
| } | ||
| CellarV2(call) => { | ||
| if call.function.is_none() { | ||
| return Err(ErrorKind::Http.context("empty function data").into()); | ||
| } | ||
|
|
||
| cellar_v2::get_encoded_call(call.function.unwrap(), request.cellar_id) | ||
| } | ||
| CellarV22(call) => { | ||
| if call.call_type.is_none() { | ||
| return Err(ErrorKind::Http.context("empty function data").into()); | ||
| } | ||
|
|
||
| cellar_v2_2::get_encoded_call(call.call_type.unwrap(), request.cellar_id) | ||
| } | ||
| CellarV25(call) => { | ||
| if call.call_type.is_none() { | ||
| return Err(ErrorKind::Http.context("empty function data").into()); | ||
| } | ||
|
|
||
| cellar_v2_5::get_encoded_call(call.call_type.unwrap(), request.cellar_id) | ||
| } | ||
| }?; | ||
|
|
||
| let encoded_call: String = ethers::types::Bytes::from(call_data).encode_hex(); | ||
|
|
||
| Ok(encoded_call) | ||
| } |
There was a problem hiding this comment.
Optimize get_encoded_call function.
The function handles various types of calls but can be optimized by reducing redundancy and improving error handling. Consider using a more generic approach to handle different call types.
+ // Consider using a strategy pattern to handle different call typesCommittable suggestion was skipped due to low confidence.
| /// Start the Encode server | ||
| #[derive(Command, Debug, Parser)] | ||
| #[clap(long_about = "DESCRIPTION\n\n")] | ||
| pub struct StartCmd {} |
There was a problem hiding this comment.
Clarify the command description for better user understanding.
The clap attribute uses a placeholder for the long description. It would be beneficial to provide a more detailed description of what starting the encode server entails.
- #[clap(long_about = "DESCRIPTION\n\n")]
+ #[clap(long_about = "Starts the encoding server which listens for encode requests and processes them.")]Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Start the Encode server | |
| #[derive(Command, Debug, Parser)] | |
| #[clap(long_about = "DESCRIPTION\n\n")] | |
| pub struct StartCmd {} | |
| #[clap(long_about = "Starts the encoding server which listens for encode requests and processes them.")] |
| abscissa_tokio::run_with_actix(&APP, async { | ||
| start_encode_server().await; | ||
| }) | ||
| .unwrap_or_else(|e| { | ||
| status_err!("executor exited with error: {}", e); | ||
| std::process::exit(1); | ||
| }); |
There was a problem hiding this comment.
Consider handling possible errors more gracefully.
The use of unwrap_or_else for error handling in a production environment might not be the best approach as it forces the application to exit. Consider logging the error and attempting a graceful shutdown or restart mechanism.
- .unwrap_or_else(|e| {
- status_err!("executor exited with error: {}", e);
- std::process::exit(1);
- });
+ .unwrap_or_else(|e| {
+ error!("executor exited with error: {}", e);
+ // Implement retry logic or a more graceful shutdown here
+ });Committable suggestion was skipped due to low confidence.
cbrit
changed the title
go/client_example/main.go
Outdated
| func main() { | ||
| /// 1. Build credentials object for two-way TLS authentication | ||
|
|
||
| // Replace these paths with your own client cert and key, and server CA paths | ||
| creds, err := buildCredentials("client.crt", "client.key", "server_ca.crt") | ||
| if err != nil { | ||
| fmt.Printf("failed to build credentials: %v", err) | ||
| return | ||
| } | ||
|
|
||
| /// 2. Construct context and client | ||
|
|
||
| conn, client := buildClient(creds) | ||
| defer conn.Close() | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
|
|
||
| /// 3. Construct adaptor calls and final request using the builder package | ||
|
|
||
| request := buildScheduleRequest() | ||
|
|
||
| /// 4. Send the request | ||
|
|
||
| _, err = client.Schedule(ctx, request) | ||
| } |
There was a problem hiding this comment.
Review main function for proper error handling and resource management.
The main function correctly builds credentials and a client, then sends a request. However, ensure that the deferred conn.Close() is correctly placed after error checks to avoid potential nil dereference.
+ if conn != nil {
+ defer conn.Close()
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func main() { | |
| /// 1. Build credentials object for two-way TLS authentication | |
| // Replace these paths with your own client cert and key, and server CA paths | |
| creds, err := buildCredentials("client.crt", "client.key", "server_ca.crt") | |
| if err != nil { | |
| fmt.Printf("failed to build credentials: %v", err) | |
| return | |
| } | |
| /// 2. Construct context and client | |
| conn, client := buildClient(creds) | |
| defer conn.Close() | |
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | |
| defer cancel() | |
| /// 3. Construct adaptor calls and final request using the builder package | |
| request := buildScheduleRequest() | |
| /// 4. Send the request | |
| _, err = client.Schedule(ctx, request) | |
| } | |
| func main() { | |
| /// 1. Build credentials object for two-way TLS authentication | |
| // Replace these paths with your own client cert and key, and server CA paths | |
| creds, err := buildCredentials("client.crt", "client.key", "server_ca.crt") | |
| if err != nil { | |
| fmt.Printf("failed to build credentials: %v", err) | |
| return | |
| } | |
| /// 2. Construct context and client | |
| conn, client := buildClient(creds) | |
| if conn != nil { | |
| defer conn.Close() | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | |
| defer cancel() | |
| /// 3. Construct adaptor calls and final request using the builder package | |
| request := buildScheduleRequest() | |
| /// 4. Send the request | |
| _, err = client.Schedule(ctx, request) | |
| } |
Adds an server that can be run with
steward encode startto submit contract call data params in the form of protos and get back the hex-encoded call data.Summary by CodeRabbit
New Features
Documentation
Refactor