Skip to content
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

refactor(core/mercury): separate upy args parsing #4182

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

obrusvit
Copy link
Contributor

@obrusvit obrusvit commented Sep 15, 2024

This PR separates the Rust flows from micropython layer.

The idea is to do the uPy argument parsing in ui/model_mercury/layout.rs and then pass them as regular arguments to code in ui/model_mercury/flow/**.rs, i.e. they won't be passed as args: &[Obj], kwargs: &Map. The goal is to keep the rust flows independent from micropython sub-crate. This separation is done for two reasons:

  • it simplifies the code in for flows creation which
  • it paves the way for apps done completely in Rust

This PR has no impact on UI or features.

TODO:

  • some lifetime issues to FIX
  • there is a few places with the usage of Obj and IterBuf
    • last place is get_address.rs, postponing this for now

@obrusvit obrusvit added code Code improvements T3T1 labels Sep 15, 2024
@obrusvit obrusvit self-assigned this Sep 15, 2024
@obrusvit
Copy link
Contributor Author

Similar thing was done here for bootloader and "common". Need to take a look how to take the same/similar approach for firmware. #3658

@obrusvit obrusvit force-pushed the obrusvit/mercury/refactor-rust-layout-upy-parsing branch 2 times, most recently from 7b39754 to d4a159d Compare September 27, 2024 15:51
- removes 2nd definition of ConfirmBlobParams, the choice of fields and
what supplied in ctor and what in `with_` methods should be thought
through again.

[no changelog]
@obrusvit obrusvit force-pushed the obrusvit/mercury/refactor-rust-layout-upy-parsing branch from d4a159d to b616b27 Compare September 30, 2024 10:26
Supply the rust layout with dedicated paremeter type instead of plain
micropython::Obj. The types used are ConfirmBlobParams and
ShowInfoParams.

[no changelog]
@obrusvit obrusvit force-pushed the obrusvit/mercury/refactor-rust-layout-upy-parsing branch from b616b27 to 58347bc Compare September 30, 2024 10:28
@obrusvit obrusvit marked this pull request as ready for review September 30, 2024 10:30
@obrusvit obrusvit requested review from mmilata and removed request for prusnak September 30, 2024 10:30
@obrusvit
Copy link
Contributor Author

obrusvit commented Sep 30, 2024

The only conflict with #3686 should be: Layout::new moved to layout.rs and renamed to Layout::new_root

Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrote some thoughts inline but overall this should be a harmless refactor that can be merged when CI passes.

case_sensitive: bool,
account: Option<TString<'static>>,
path: Option<TString<'static>>,
xpubs: Obj, // TODO: get rid of Obj
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this can use some kind of closure?

@@ -1540,16 +1866,16 @@ pub static mp_module_trezorui2: Module = obj_module! {
/// br_code: ButtonRequestType,
/// br_name: str,
/// ) -> LayoutObj[tuple[UiResult, int]]:
/// """Numer input with + and - buttons, description, and context menu with cancel and
/// """Numebr input with + and - buttons, description, and context menu with cancel and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// """Numebr input with + and - buttons, description, and context menu with cancel and
/// """Number input with + and - buttons, description, and context menu with cancel and

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Code improvements T3T1
Projects
Status: 🏃‍♀️ In progress
Development

Successfully merging this pull request may close these issues.

2 participants