-
Notifications
You must be signed in to change notification settings - Fork 80
feat: implement CliClient #1642
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
base: main
Are you sure you want to change the base?
Conversation
…n with CLI config
| // Clean up any existing global config | ||
| cleanup_global_config(); |
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.
These home dirs are process-global environments, right? So depending on how nextest runs tests there could be cases where this messes up with some other tests, is that correct? If so, I wonder if we should run these serially.
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.
Good catch! You're absolutely right, these tests manipulate process-global resources (~/.miden home directory and env::set_current_dir()). I've added #[serial_test::serial(global_config)] to all four tests to ensure they run serially and avoid race conditions with nextest or parallel test execution.
bin/miden-cli/tests/cli.rs
Outdated
| // Verify the client is functional by syncing | ||
| let sync_result = client.sync_state().await; | ||
| assert!(sync_result.is_ok(), "Client sync failed: {:?}", sync_result.err()); |
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.
I think this doesn't really assert that the test ends up using the local config (although this might not be simple anyway)
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.
Same applies for the global one
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.
Yeah I think unfortuantely there is no good way to test this. The assertions didn't actually verify which config was used.
I've fixed this by looking at the actual store file path that was created.
bin/miden-cli/tests/cli.rs
Outdated
| // Attempt to create a client (should fail) | ||
| let client = miden_client_cli::CliClient::from_system_user_config(DebugMode::Disabled).await; | ||
|
|
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.
Should this not silently initialize the client? Or is that not ideal?
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.
Absolutely correct, I missed that one. I implemented that now!
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.
Updated the implementation to call InitCmd::default().execute() when no config is found, and adjusted the test to verify silent initialization works correctly.
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub async fn from_system_user_config(debug_mode: DebugMode) -> Result<Self, CliError> { |
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.
One thing that would be nice is passing a CliConfig to a client constructor, and then have different constructors/loaders for CliConfig (for example, one for local config, one for global, one that respects the priority); it can also save the TOML automatically on the correct directories. We can still keep this from_system_user_config() as well.
I think this also makes tests better in the sense that you don't have to change the working dir.
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.
I completely agree with your suggestion! However, are you fine if we move this to a separate issue to speed up this PR and the mint command (mint has taken quite long already)?
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.
Nevermind, I addressed this too and refactored the CLI config
bin/miden-cli/tests/cli.rs
Outdated
| // Change to the temp directory to ensure the local config is picked up | ||
| let original_dir = env::current_dir().unwrap(); | ||
| env::set_current_dir(&temp_dir)?; |
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.
I think this is also process-wide which could be problematic
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.
Fixed according with #1642 (comment)
feat(cli): add silent initialization for CliClient test(cli): refactor tests not to use global resources test(cli): modify from_system_user_config CliClient tests to run serially test(cli): modify from_system_user_config tests to effectively assert global and local config prioritization test(cli): add silent initialization tests for from_system_user_config
UpdateHey everyone, I made a commit with several changes to address the code feedback from @igamigo . Here's what changed:
|
igamigo
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.
LGTM. I left some comments, most of which are minor. If you prefer to tackle them separately, let's open an issue for them before merging this.
bin/miden-cli/src/utils.rs
Outdated
| /// | ||
| /// # Deprecated | ||
| /// This function is deprecated in favor of `CliConfig::from_system()` which provides | ||
| /// the same functionality with a cleaner API. | ||
| pub(super) fn load_config_file() -> Result<(CliConfig, PathBuf), CliError> { |
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.
I think we can just delete this and use the new functions instead.
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.
Done! Deleted load_config_file() from utils.rs and replaced all usages with CliConfig::from_system() directly
bin/miden-cli/src/lib.rs
Outdated
| /// | ||
| /// # Returns | ||
| /// | ||
| /// A configured `CliClient` instance. |
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.
nit: These can be doc-linked if you use [] instead:
| /// A configured `CliClient` instance. | |
| /// A configured [`CliClient`] instance. |
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.
This happens throughout the file. It's not critical though so if you prefer the current state that's fine too
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.
Good catch! Updated all documentation to use proper doc-links like [CliClient], [CliConfig],[CliError],[DebugMode::Enabled], etc. Also added cross-references between related methods.
| // Try local first | ||
| match Self::from_local_dir() { | ||
| Ok(config) => Ok(config), | ||
| Err(_) => { | ||
| // Fall back to global | ||
| Self::from_global_dir().map_err(|_| { | ||
| CliError::Config( | ||
| "No configuration file found".to_string().into(), | ||
| "Neither local nor global config file exists. Run 'miden-client init' to create one.".to_string() | ||
| ) | ||
| }) | ||
| }, | ||
| } |
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.
This is fine, but it will also silently eat any errors that are not "file not found" too. For example, if I have a local config but for some reason it fails to parse correctly, it will go through the Err(_) branch and load the global config and this could be confusing. I believe we should check that the file has not been found instead of matching for any error, what do you think?
Maybe it could be addressed on a different issue unless you prefer doing it here.
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.
Great point! Added a new CliError::ConfigNotFound variant specifically for "file not found" errors. Now from_system() only falls back to global config when encountering ConfigNotFound, parse errors and other issues are propagated immediately
|
|
||
| // FROM_SYSTEM_USER_CONFIG TESTS | ||
| // ================================================================================================ | ||
| /// Tests that `CliClient::from_system_user_config()` successfully creates a client with the same |
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.
I don't see a call to CliClient::from_system_user_config in this test
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.
Fixed! The test now properly changes the working directory to the temp directory and calls CliClient::from_system_user_config() instead of from_config(). Also added cleanup_global_config() for proper isolation.
|
One more small thing: For users to make use of client structs and code, I think we'll want to re-export everything from |
Added |
feat(cli): add ConfigNotFound error variant for better error handling feat(cli): re-export miden_client as client module docs(cli): use doc-links for type references in documentation fix(cli): update test to call from_system_user_config
I think it might be better to do |
Updates
There has been the following update since the beginning of this PR: #1642 (comment)
Summary
Closes #1639
This PR implements a
from_system_user_config()method forCliClientthat creates a Client instance using the same configuration as the CLI tool (reading from.miden/miden-client.tomlor local config). This enables anyone to programmatically obtain a properly configured CLI client.This change is required for implementing the mint command https://github.com/0xMiden/midenup/issues/128, in particular with the new implementation strategy described in 0xMiden/midenup#130 (comment). The https://github.com/0xMiden/miden-faucet repository needs to create a CLI-configured client instance to consume notes and sync state after making the mint request when using the miden-faucet-client, see this PR description for more details: 0xMiden/miden-faucet#196.
Details
New
CliClientwrapper typeA newtype wrapper
CliClient(Client<CliKeyStore>)has been added tomiden-client-cliwithDerefandDerefMutimplementations. This allows external projects to:CliClient::from_system_user_config()Clientmethods transparently through deref coercionClient<CliKeyStore>when needed viainto_inner(),inner(), orinner_mut()Configuration loading behavior
The
from_system_user_config()method searches for configuration files in order:.miden/miden-client.tomlin the current working directory.miden/miden-client.tomlin the home directoryThe client is initialized with the same settings as the CLI:
DebugModeenum, matchingClientBuilder::in_debug_mode()API)The new
CliClienthas been integrated into the existing CLI logic to ensure that the CliClient is the single source of configuration for the CLI client instance.Usage example
This pattern was originally described by @igamigo in the following comment: 0xMiden/midenup#130 (comment)