-
Notifications
You must be signed in to change notification settings - Fork 123
Remove network runnable abstraction. #2364
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request removes the NetworkRunnable trait abstraction and replaces it with direct execute methods on command structs. The goal is to simplify the code and make control flow easier to follow.
Changes:
- Removed the
NetworkRunnabletrait definition and all its implementations - Replaced
run_against_rpc_servermethod withexecutemethod for each command - Updated all commands to pass parameters directly instead of using Option wrappers
- Updated test utilities to call
executemethods directly - Refactored two unrelated methods in container/start.rs for better readability
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/soroban-cli/src/commands/mod.rs | Removes NetworkRunnable trait definition |
| cmd/soroban-cli/src/commands/tx/simulate.rs | Converts to direct execute method, removes async_trait |
| cmd/soroban-cli/src/commands/tx/send.rs | Converts to execute method with explicit config construction |
| cmd/soroban-cli/src/commands/events.rs | Converts to execute method with explicit config construction |
| cmd/soroban-cli/src/commands/contract/upload.rs | Converts to execute with direct boolean parameters |
| cmd/soroban-cli/src/commands/contract/restore.rs | Converts to execute with direct boolean parameters |
| cmd/soroban-cli/src/commands/contract/read.rs | Converts to execute method with simplified network resolution |
| cmd/soroban-cli/src/commands/contract/invoke.rs | Converts to execute with direct parameters, changes global_args passing |
| cmd/soroban-cli/src/commands/contract/fetch.rs | Converts to execute method with explicit config construction |
| cmd/soroban-cli/src/commands/contract/extend.rs | Converts to execute with direct boolean parameters |
| cmd/soroban-cli/src/commands/contract/deploy/wasm.rs | Converts to execute, updates upload command invocation |
| cmd/soroban-cli/src/commands/contract/deploy/asset.rs | Converts to execute with direct boolean parameters |
| cmd/soroban-cli/src/commands/contract/bindings/typescript.rs | Simplifies to execute method with single quiet parameter |
| cmd/soroban-cli/src/commands/container/start.rs | Refactors helper methods to use early returns (unrelated change) |
| cmd/crates/soroban-test/tests/it/util.rs | Updates test to call execute directly |
| cmd/crates/soroban-test/tests/it/integration/util.rs | Updates test to call execute directly |
| cmd/crates/soroban-test/tests/it/integration/hello_world.rs | Updates tests to call execute directly |
| cmd/crates/soroban-test/tests/it/integration/bindings.rs | Updates tests to call execute directly |
| cmd/crates/soroban-test/src/lib.rs | Removes run_cmd_with helper, updates cmd_with_config signature |
leighmcculloch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple questions but this looks good regardless 😎.
What
Remove network runnable abstraction in favor of a more direct approach.
Why
So it's easier to follow.
Known limitations
N/A