Feature/mintlayer app #1
Conversation
Upgrade to SDK 0.2.0
`from_bip32` is now `derive_from_path`.
[fix] Use SeedDerive trait
Add pending message support
This reverts commit f72366c.
`cargo-ledger` update
Update to latest SDK
use dedicated sections for each device
Moving to new manifest format
OBorce
force-pushed
the
feature/mintlayer-app
branch
7 times, most recently
from
990853e to
a099ace
Compare
OBorce
force-pushed
the
feature/mintlayer-app
branch
from
a099ace to
7c1d9f4
Compare
There was a problem hiding this comment.
So, my main concerns are:
- The protocol specification is spread over 2 repos. Also, a lot of magic numbers are used. Also, you often construct commands and parse responses "by hand", by reading/writing pieces of a u8 array.
IMO the specification should be in one place (in the ledger app repo, in a separate crate), all constants should have names, the commands and responses should be typed, i.e. be Rust structs that are encoded when sent and decoded when received. - We need to decide whether/how the ml core primitives should be used by hw wallet firmwares.
I've mentioned my suggestions in one of the comments below.
| # parity-scale-codec = { version = "3.7", default-features = false, features = [ | ||
| # "derive", | ||
| # "chain-error", | ||
| # ] } | ||
| parity-scale-codec = { git = "https://github.com/OBorce/parity-scale-codec.git", branch = "fix/missing-target-atomic-ptr", default-features = false, features = [ | ||
| "derive", | ||
| "chain-error", | ||
| ] } | ||
| ml_common = { git = "https://github.com/mintlayer/mintlayer-core", package = "trezor-common", branch = "feature/hardware-wallet-common2" } |
There was a problem hiding this comment.
Plz mention the PR that fixes the issue and/or the parity-scale-codec's release where it's supposed to be included (I guess anything after the current v3.7.5 will do).
src/handlers/sign_tx.rs
Outdated
| | TxOutput::LockThenTransfer(value, _, _) | ||
| | TxOutput::Htlc(value, _) => { | ||
| let (coin_or_token_id, amount) = into_coin_or_token_id_and_amount(value)?; | ||
| increase_input_totals(&mut ctx.total_inputs, coin_or_token_id, amount)?; |
There was a problem hiding this comment.
I don't like the fact that you use input commitments data for something other than calculating the commitments themselves (like updating total inputs), because there is no guarantee (conceptually) that, for example, the commitments will contain all UTXOs.
The better approach would be, like for Trezor, to receive all the necessary data (UTXOs, order infos etc) separately and then use it to construct the commitments.
| fn handle_apdu(comm: &mut Comm, ins: &Instruction, ctx: &mut Context) -> Result<(), AppSW> { | ||
| match ins { | ||
| Instruction::GetAppName => { | ||
| comm.append(env!("CARGO_PKG_NAME").as_bytes()); | ||
| Ok(()) | ||
| } | ||
| Instruction::GetVersion => handler_get_version(comm), | ||
| Instruction::GetPubkey { display } => handler_get_public_key(comm, *display), | ||
| Instruction::SignTx { p1, more } => handler_sign_tx(comm, *p1, *more, &mut ctx.data), | ||
| Instruction::SignMessage { chunk, more } => { | ||
| handler_sign_message(comm, *chunk, *more, &mut ctx.data) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Plz separate APDU handling from the command handling.
- (probably) rename Instruction to Command.
- As I've already said in other places, each command should better have the corresponding Data and Response structs that implement Encode/Decode. So, the
Instruction/Commandstruct would contain the already decoded XXXCommandData struct and the handlers would return XXXCommandResponse. So, the handlers will need neitherCommnorContext.
P.S. the names like handler_get_version etc look strange. Perhaps the functions should be named e.g. get_version_command_handler or handle_get_version/handle_get_version_command.
| Instruction::GetAppName => { | ||
| comm.append(env!("CARGO_PKG_NAME").as_bytes()); | ||
| Ok(()) | ||
| } | ||
| Instruction::GetVersion => handler_get_version(comm), |
There was a problem hiding this comment.
I don't think we need GetAppName, there is already a "standard" way to check the current app name, see my comment in the core repo's PR.
Regarding the version - the simple major.minor.patch should already be returned by the same "standard" APDU that returns the name (as I understand, it's taken from the package version, while the name is the "name" in "[package.metadata.ledger]").
But IMO it's better to support full semantic versioning, i.e. also return the "prerelease id" and the "build metadata".
In the Trezor FW the "prerelease id" is currently empty (its supposed values are "alpha"/"beta" etc) and the "build metadata" is the repo commit hash. Obtaining git repo hash shouldn't be hard (and we already have build.rs in this project), so it'd be better to return it as part of GetAppInfo as well.
So I suggest keeping GetVersion (though it can probably be generalized to GetAppInfo) but returning the full semantic version instead.
src/main.rs
Outdated
| } | ||
|
|
||
| impl TryFrom<u8> for P1SignTx { | ||
| type Error = AppSW; |
There was a problem hiding this comment.
I don't think that using the status word as Error is a good idea (especially if we move all protocol-related stuff to this repo, which means that some Error-returning functions will be called from the host where status words make no sense).
So I'd suggest to introduce a normal error enum and then implement conversion from the error to status word.
OBorce
force-pushed
the
feature/mintlayer-app
branch
14 times, most recently
from
7ce6d0f to
ab1fa35
Compare
OBorce
force-pushed
the
feature/mintlayer-app
branch
from
ab1fa35 to
877082e
Compare
19 participants
No description provided.