feat: Add unified cross-platform Ledger installer (Fixes #7795)#7803
feat: Add unified cross-platform Ledger installer (Fixes #7795)#7803taiwofelix35 wants to merge 1 commit into
Conversation
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
There was a problem hiding this comment.
Code Review
This pull request introduces a unified, cross-platform installer for the Minotari Ledger Wallet, featuring a core Python script with automated device detection and release management, supported by platform-specific wrappers and documentation. Feedback focuses on improving the robustness of command execution by using sys.executable -m for ledgerctl, addressing a potential path traversal vulnerability during ZIP extraction, and correcting test cases that currently fail to exercise the actual implementation logic.
|
|
||
| try: | ||
| result = subprocess.run( | ||
| ["ledgerctl", "list"], |
There was a problem hiding this comment.
Calling ledgerctl directly assumes it is already in the system's PATH. Since this script installs ledgerctl via pip in the same execution, the PATH might not be updated yet, or the user's environment might not include the Python scripts directory (common on Windows). It is more robust to invoke it using sys.executable -m ledgerctl.
| ["ledgerctl", "list"], | |
| [sys.executable, "-m", "ledgerctl", "list"], |
| print_info(f"Installing app from {os.path.basename(app_json_path)}...") | ||
|
|
||
| try: | ||
| result = subprocess.run(["ledgerctl", "install", app_json_path], timeout=120) |
There was a problem hiding this comment.
Similar to the detection step, it is safer to invoke ledgerctl via sys.executable -m to ensure the command is found regardless of whether the scripts directory is in the user's PATH.
| result = subprocess.run(["ledgerctl", "install", app_json_path], timeout=120) | |
| result = subprocess.run([sys.executable, "-m", "ledgerctl", "install", app_json_path], timeout=120) |
|
|
||
| try: | ||
| with zipfile.ZipFile(zip_path, "r") as zip_ref: | ||
| zip_ref.extractall(extract_dir) |
There was a problem hiding this comment.
Using zip_ref.extractall() without validating the paths of the files inside the archive can lead to a "Zip Slip" vulnerability (path traversal). While the source is a trusted repository, it is a security best practice to verify that all extracted files are within the target directory before extraction.
| def test_parse_release_json(self, mock_urlopen): | ||
| mock_response = MagicMock() | ||
| mock_response.read.return_value = json.dumps( | ||
| { | ||
| "assets": [ | ||
| { | ||
| "name": "minotari_ledger_wallet-flex.zip", | ||
| "browser_download_url": "https://example.com/flex.zip", | ||
| }, | ||
| { | ||
| "name": "minotari_ledger_wallet-nanox.zip", | ||
| "browser_download_url": "https://example.com/nanox.zip", | ||
| }, | ||
| ] | ||
| } | ||
| ).encode() | ||
|
|
||
| mock_urlopen.return_value.__enter__.return_value = mock_response | ||
|
|
||
| with patch("urllib.request.urlretrieve"): | ||
| with patch("os.path.join", return_value="/tmp/test.zip"): | ||
| try: | ||
| data = json.loads(mock_response.read().decode()) | ||
| assets = [ | ||
| asset | ||
| for asset in data["assets"] | ||
| if "minotari_ledger_wallet-flex" in asset["name"] | ||
| ] | ||
| self.assertEqual(len(assets), 1) | ||
| self.assertEqual(assets[0]["name"], "minotari_ledger_wallet-flex.zip") | ||
| except json.JSONDecodeError: | ||
| self.fail("Failed to parse JSON response") |
There was a problem hiding this comment.
This test case does not actually call installer.download_release(). Instead, it duplicates some of the logic from that function and asserts on local variables. This means the actual implementation in install_minotari_ledger.py remains untested. The test should be refactored to call the target function and verify its behavior.
| def test_extract_zip(self, mock_zipfile): | ||
| mock_zip_instance = MagicMock() | ||
| mock_zipfile.return_value.__enter__.return_value = mock_zip_instance | ||
|
|
||
| import tempfile | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| test_zip = os.path.join(tmpdir, "test.zip") | ||
| Path(test_zip).touch() | ||
| self.assertTrue(os.path.exists(test_zip)) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Pull request overview
This PR introduces a unified, cross-platform (Windows/macOS/Linux) installer workflow for the Minotari Ledger Wallet app, including a Python-based installer plus thin Bash/PowerShell wrappers and a lightweight unit test suite.
Changes:
- Added
install_minotari_ledger.pyto detect Ledger model, download the matching GitHub release asset, extract the manifest, and install vialedgerctl. - Added platform wrappers (
install_minotari_ledger.sh,install_minotari_ledger.ps1) and a dedicated usage guide (README_UNIFIED_INSTALLER.md). - Added a new
unittest-based test suite (test_installer.py) intended to cover detection/selection/error handling.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| applications/minotari_ledger_wallet/wallet/install_minotari_ledger.py | Core unified installer logic: dependency setup, model detection, release download/extract, and install via ledgerctl. |
| applications/minotari_ledger_wallet/wallet/install_minotari_ledger.sh | Bash wrapper to run the Python installer on macOS/Linux. |
| applications/minotari_ledger_wallet/wallet/install_minotari_ledger.ps1 | PowerShell wrapper to run the Python installer on Windows. |
| applications/minotari_ledger_wallet/wallet/README_UNIFIED_INSTALLER.md | Documentation for running the unified installer and troubleshooting. |
| applications/minotari_ledger_wallet/wallet/test_installer.py | Unit tests for key installer functions (currently with gaps and a few reliability issues). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| __import__(package) | ||
| print_success(f"{package} already installed") | ||
| except ImportError: | ||
| print_info(f"Installing {package}...") | ||
| subprocess.check_call([sys.executable, "-m", "pip", "install", package]) | ||
| print_success(f"{package} installed") | ||
|
|
||
|
|
| if not check_command("pip") and not check_command("pip3"): | ||
| print_error("pip not found. Install Python with pip enabled.") |
| try: | ||
| result = subprocess.run( | ||
| ["ledgerctl", "list"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, | ||
| ) |
| zip_ref.extractall(extract_dir) | ||
|
|
| mock_version = MagicMock() | ||
| mock_version.major = 3 | ||
| mock_version.minor = 6 | ||
| mock_version.__lt__ = lambda self, other: (3, 6) < other | ||
|
|
||
| with patch("sys.version_info", mock_version): |
| class TestGitHubReleaseDownload(unittest.TestCase): | ||
| @patch("urllib.request.urlopen") | ||
| def test_parse_release_json(self, mock_urlopen): | ||
| mock_response = MagicMock() | ||
| mock_response.read.return_value = json.dumps( | ||
| { | ||
| "assets": [ | ||
| { | ||
| "name": "minotari_ledger_wallet-flex.zip", | ||
| "browser_download_url": "https://example.com/flex.zip", | ||
| }, | ||
| { | ||
| "name": "minotari_ledger_wallet-nanox.zip", | ||
| "browser_download_url": "https://example.com/nanox.zip", | ||
| }, | ||
| ] | ||
| } | ||
| ).encode() | ||
|
|
||
| mock_urlopen.return_value.__enter__.return_value = mock_response | ||
|
|
||
| with patch("urllib.request.urlretrieve"): | ||
| with patch("os.path.join", return_value="/tmp/test.zip"): | ||
| try: | ||
| data = json.loads(mock_response.read().decode()) | ||
| assets = [ | ||
| asset | ||
| for asset in data["assets"] | ||
| if "minotari_ledger_wallet-flex" in asset["name"] | ||
| ] | ||
| self.assertEqual(len(assets), 1) | ||
| self.assertEqual(assets[0]["name"], "minotari_ledger_wallet-flex.zip") | ||
| except json.JSONDecodeError: | ||
| self.fail("Failed to parse JSON response") | ||
|
|
| @patch("zipfile.ZipFile") | ||
| def test_extract_zip(self, mock_zipfile): | ||
| mock_zip_instance = MagicMock() | ||
| mock_zipfile.return_value.__enter__.return_value = mock_zip_instance | ||
|
|
||
| import tempfile | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| test_zip = os.path.join(tmpdir, "test.zip") | ||
| Path(test_zip).touch() | ||
| self.assertTrue(os.path.exists(test_zip)) | ||
|
|
||
|
|
| The old per-model installation scripts are available in separate directories: | ||
| - flex/ - Ledger Flex | ||
| - nanosplus/ - Ledger Nano S Plus | ||
| - nanox/ - Ledger Nano X | ||
| - stax/ - Ledger Stax | ||
|
|
||
| Each has: | ||
| - install_minotari_ledger_[model].sh (macOS/Linux) | ||
| - install_ledger_win.ps1 (Windows) | ||
|
|
||
| These are maintained for compatibility but the unified installer is recommended. | ||
|
|
|
|
||
| ### ledgerctl not found after installation | ||
| - The installer should auto-install ledgerctl via pip | ||
| - If issues persist, try: pip install ledgerctl |
| def install_dependencies() -> None: | ||
| required_packages = ["protobuf", "setuptools", "ecdsa", "ledgerwallet", "ledgerctl"] | ||
|
|
||
| print_info("Installing Python dependencies...") | ||
| for package in required_packages: | ||
| try: | ||
| __import__(package) | ||
| print_success(f"{package} already installed") | ||
| except ImportError: | ||
| print_info(f"Installing {package}...") | ||
| subprocess.check_call([sys.executable, "-m", "pip", "install", package]) | ||
| print_success(f"{package} installed") |
SWvheerden
left a comment
There was a problem hiding this comment.
I believe the co-pilot and Gemini stuff is relevant
Take a look at the current install script to see how its done
Description
This PR adds a unified cross-platform installer for the Minotari Ledger Wallet that auto-detects the connected Ledger model and installs the correct app in one step.
Closes
Fixes #7795
What’s Included
Testing
python applications/minotari_ledger_wallet/wallet/test_installer.py