Merge pull request #373 from LedgerHQ/lp/ci

[Python package] Clearer code?

.github/workflows/continuous-integration-workflow.yml

@@ -14,34 +14,30 @@ on:
 jobs:
 
   linter:
-    name: Linter on C & Python code
+    name: Linter on C code
     runs-on: ubuntu-latest
     steps:
     - name: Clone
-      uses: actions/checkout@v2
+      uses: actions/checkout@v3
       with:
         fetch-depth: 0
-    - name: Python dependency
-      run: pip install flake8
     - name: Lint C code
-      uses: DoozyX/clang-format-lint-action@v0.13
+      uses: DoozyX/clang-format-lint-action@v0.16.1
       with:
         source: 'src tests'
         extensions: 'c,h'
         clangFormatVersion: 11
-    - name: Lint Python code
-      run: find . -type f -name '*.py' -exec flake8 --max-line-length=120 '{}' '+'
 
   misspell:
     name: Check misspellings
     runs-on: ubuntu-latest
     steps:
     - name: Clone
-      uses: actions/checkout@v2
+      uses: actions/checkout@v3
       with:
         fetch-depth: 0
     - name: Check misspellings
-      uses: codespell-project/actions-codespell@de089481bd65b71b4d02e34ffb3566b6d189333e
+      uses: codespell-project/actions-codespell@v1
       with:
         builtin: clear,rare
         check_filenames: true
@@ -56,7 +52,7 @@ jobs:
       image: docker://ghcr.io/ledgerhq/speculos-builder:latest
     steps:
     - name: Clone
-      uses: actions/checkout@v2
+      uses: actions/checkout@v3
       with:
         fetch-depth: 0
     - name: Rebuild with code coverage instrumentation
@@ -89,7 +85,7 @@ jobs:
 
     steps:
     - name: Clone
-      uses: actions/checkout@v2
+      uses: actions/checkout@v3
       with:
         fetch-depth: 0
 
@@ -114,7 +110,7 @@ jobs:
 
     steps:
     - name: Clone
-      uses: actions/checkout@v2
+      uses: actions/checkout@v3
       with:
         fetch-depth: 0
 
@@ -196,7 +192,7 @@ jobs:
     needs: [build, coverage, package_and_test_docker]
     steps:
     - name: Clone
-      uses: actions/checkout@v2
+      uses: actions/checkout@v3
       with:
         fetch-depth: 0
     - name: Build and publish to GitHub Packages

.github/workflows/python_checks.yml

@@ -0,0 +1,43 @@
+name: Python checks
+
+on:
+  workflow_dispatch:
+  push:
+    branches:
+      - master
+      - develop
+  pull_request:
+
+jobs:
+  linter:
+    name: Linter on Python code
+    runs-on: ubuntu-latest
+    steps:
+    - name: Clone
+      uses: actions/checkout@v3
+      with:
+        fetch-depth: 0
+    - name: Python dependency
+      run: pip install flake8
+    - name: Lint Python code
+      run: find speculos/ -type f -name '*.py' -exec flake8 --max-line-length=120 '{}' '+'
+
+  mypy:
+    name: Type checking
+    runs-on: ubuntu-latest
+    steps:
+    - name: Clone
+      uses: actions/checkout@v3
+    - run: pip install mypy types-requests types-setuptools PyQt5-stubs
+    - name: Mypy type checking
+      run: mypy speculos
+
+  bandit:
+    name: Security checking
+    runs-on: ubuntu-latest
+    steps:
+    - name: Clone
+      uses: actions/checkout@v3
+    - run: pip install bandit
+    - name: Bandit security checking
+      run: bandit -r speculos -ll || echo 0

.github/workflows/reusable_ragger_tests_latest_speculos.yml

@@ -28,11 +28,39 @@ jobs:
       app_branch_name: ${{ inputs.app_branch_name }}
       upload_app_binaries_artifact: "compiled_app_binaries"
 
+  build_docker_image:
+    name: Build Speculos Docker image
+    runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+    steps:
+    - name: Clone
+      uses: actions/checkout@v3
+      with:
+          ref: ${{ inputs.speculos_app_branch_name }}
+          submodules: recursive
+          fetch-depth: 0
+
+    - name: Set up Docker Buildx
+      uses: docker/setup-buildx-action@v2
+
+    - name: Build the Speculos docker
+      uses: docker/build-push-action@v4
+      with:
+        push: false
+        tags: ledgerhq/speculos:test
+        context: .
+        outputs: type=docker,dest=/tmp/speculos_image.tar
+    - name: Upload artifact
+      uses: actions/upload-artifact@v3
+      with:
+        name: speculos_image
+        path: /tmp/speculos_image.tar
+
   ragger_tests:
     name: Functional tests with Ragger
     runs-on: ubuntu-latest
-    container:
-      image: docker://ghcr.io/ledgerhq/speculos-builder:latest
+    needs: [build_docker_image, build_application]
     strategy:
       fail-fast: false
       matrix:
@@ -50,12 +78,6 @@ jobs:
           submodules: recursive
           fetch-depth: 0
 
-    - name: Build the Speculos docker
-      uses: docker/build-push-action@v1
-      with:
-          push: false
-          tags: test
-
     - name: Clone
       uses: actions/checkout@v3
       with:
@@ -65,6 +87,20 @@ jobs:
           submodules: recursive
           fetch-depth: 0
 
+    - name: Set up Docker Buildx
+      uses: docker/setup-buildx-action@v2
+
+    - name: Download artifact
+      uses: actions/download-artifact@v3
+      with:
+        name: speculos_image
+        path: /tmp
+
+    - name: Load image
+      run: |
+        docker load --input /tmp/speculos_image.tar
+        docker image ls -a
+
     - name: Download app binaries
       uses: actions/download-artifact@v3
       with:

.github/workflows/speculos-builder.yml

@@ -20,7 +20,7 @@ jobs:
 
     steps:
     - name: Clone
-      uses: actions/checkout@v2
+      uses: actions/checkout@v3
 
     - name: Build and push speculos-builder to GitHub Packages
       uses: docker/build-push-action@v1

.gitignore

@@ -16,3 +16,4 @@ __version__.py
 *.pyc
 dist/
 *egg-info
+.coverage

speculos/api/apdu.py

@@ -31,7 +31,7 @@ def exchange(self, data: bytes) -> Generator[bytes, None, None]:
                     self.response_condition.wait()
             yield json.dumps({"data": self.response.hex()}).encode()
 
-    def seph_apdu_callback(self, data: bytes):
+    def seph_apdu_callback(self, data: bytes) -> None:
         """
         Called by seph when data is transmitted by the SE. That data should
         be the response to a prior APDU request

speculos/api/api.py

@@ -1,36 +1,41 @@
 import socket
 import threading
 import pkg_resources
-from typing import Dict, Optional
+from typing import Any, Dict
 from flask import Flask
 from flask_restful import Api
 
-from ..mcu.readerror import ReadError
-from ..mcu.seproxyhal import SeProxyHal
+from speculos.mcu.display import DisplayNotifier, IODevice
+from speculos.mcu.readerror import ReadError
+from speculos.mcu.seproxyhal import SeProxyHal
+from speculos.observer import BroadcastInterface
 from .apdu import APDU
 from .automation import Automation
 from .button import Button
-from .events import Events, EventsBroadcaster
+from .events import Events
 from .finger import Finger
 from .screenshot import Screenshot
 from .swagger import Swagger
 from .web_interface import WebInterface
 from .ticker import Ticker
 
 
-class ApiRunner:
-    """Run the Speculos API server, with a notification when it stops"""
+class ApiRunner(IODevice):
+    """Run the Speculos API server in a dedicated thread, with a notification when it stops"""
     def __init__(self, api_port: int) -> None:
-        self._app: Optional[Flask] = None
-        # self.s is used by Screen.add_notifier. Closing self._notify_exit
+        self._app: Flask
+        # self.sock is used by Screen.add_notifier. Closing self._notify_exit
         # signals it that the API is no longer running.
-        self.s: socket.socket
+        self.sock: socket.socket
         self._notify_exit: socket.socket
-        self.s, self._notify_exit = socket.socketpair()
+        self.sock, self._notify_exit = socket.socketpair()
         self.api_port: int = api_port
 
-    def can_read(self, s: int, screen) -> None:
-        assert s == self.s.fileno()
+    @property
+    def file(self):
+        return self.sock
+
+    def can_read(self, screen: DisplayNotifier) -> None:
         # Being able to read from the socket only happens when the API server exited.
         raise ReadError("API server exited")
 
@@ -45,71 +50,43 @@ def _run(self) -> None:
             self._notify_exit.close()
 
     def start_server_thread(self,
-                            screen_,
+                            screen_: DisplayNotifier,
                             seph_: SeProxyHal,
-                            automation_server: EventsBroadcaster) -> None:
+                            automation_server: BroadcastInterface) -> None:
         wrapper = ApiWrapper(screen_, seph_, automation_server)
         self._app = wrapper.app
         api_thread = threading.Thread(target=self._run, name="API-server", daemon=True)
         api_thread.start()
 
 
 class ApiWrapper:
-    def __init__(self, screen, seph: SeProxyHal, automation_server: EventsBroadcaster):
-        self._screen = screen
-        self._seph = seph
-        self._set_app()
-
-        self._api = Api(self.app)
-        self._api.add_resource(APDU, "/apdu",
-                               resource_class_kwargs=self._seph_kwargs)
-        self._api.add_resource(Automation, "/automation",
-                               resource_class_kwargs=self._seph_kwargs)
-        self._api.add_resource(Button, "/button/left", "/button/right", "/button/both",
-                               resource_class_kwargs=self._seph_kwargs)
-        self._api.add_resource(Events, "/events",
-                               resource_class_kwargs={**self._app_kwargs,
-                                                      "automation_server": automation_server})
-        self._api.add_resource(Finger, "/finger",
-                               resource_class_kwargs=self._seph_kwargs)
-        self._api.add_resource(Screenshot, "/screenshot",
-                               resource_class_kwargs=self._screen_kwargs)
-        self._api.add_resource(Swagger, "/swagger/",
-                               resource_class_kwargs=self._app_kwargs)
-        self._api.add_resource(WebInterface, "/",
-                               resource_class_kwargs=self._app_kwargs)
-        self._api.add_resource(Ticker, "/ticker/",
-                               resource_class_kwargs=self._seph_kwargs)
+    def __init__(self, screen: DisplayNotifier, seph: SeProxyHal, automation_server: BroadcastInterface):
 
-    def _set_app(self):
         static_folder = pkg_resources.resource_filename(__name__, "/static")
         self._app = Flask(__name__, static_url_path="", static_folder=static_folder)
         self._app.env = "development"
 
-    @property
-    def _screen_kwargs(self):
-        return {"screen": self.screen}
+        screen_kwargs = {"screen": screen}
+        seph_kwargs = {"seph": seph}
+        app_kwargs = {"app": self._app}
+        event_kwargs: Dict[str, Any] = {**app_kwargs, "automation_server": automation_server}
 
-    @property
-    def _app_kwargs(self) -> Dict[str, Flask]:
-        return {"app": self.app}
+        self._api = Api(self.app)
 
-    @property
-    def _seph_kwargs(self) -> Dict[str, SeProxyHal]:
-        return {"seph": self.seph}
+        self._api.add_resource(APDU, "/apdu", resource_class_kwargs=seph_kwargs)
+        self._api.add_resource(Automation, "/automation", resource_class_kwargs=seph_kwargs)
+        self._api.add_resource(Button,
+                               "/button/left",
+                               "/button/right",
+                               "/button/both",
+                               resource_class_kwargs=seph_kwargs)
+        self._api.add_resource(Events, "/events", resource_class_kwargs=event_kwargs)
+        self._api.add_resource(Finger, "/finger", resource_class_kwargs=seph_kwargs)
+        self._api.add_resource(Screenshot, "/screenshot", resource_class_kwargs=screen_kwargs)
+        self._api.add_resource(Swagger, "/swagger/", resource_class_kwargs=app_kwargs)
+        self._api.add_resource(WebInterface, "/", resource_class_kwargs=app_kwargs)
+        self._api.add_resource(Ticker, "/ticker/", resource_class_kwargs=seph_kwargs)
 
     @property
     def app(self) -> Flask:
         return self._app
-
-    @property
-    def api(self) -> Api:
-        return self._api
-
-    @property
-    def screen(self):
-        return self._screen
-
-    @property
-    def seph(self) -> SeProxyHal:
-        return self._seph