fix: hard-coded CLA byte

[xJonathanLEI]

Fixes #137.

Submitting this patch optimistically. This contains a breaking change as a new field cla has been added.

Existing downstream applications would need to set cla to 0xe0 to continue to use app-ethereum.

[xJonathanLEI]

CI failing but looks like it's due to a newer Rust version instead of my diffs.

[prestwich]

CI failing but looks like it's due to a newer Rust version instead of my diffs.

you do need to update the APDU construction in wasm.rs

That said, in what cases would we NOT know the CLA at compile-time?

[xJonathanLEI]

I guess you'd always know it compile time.

Am I understanding correctly that you're suggesting const generic?

[prestwich]

yes, if we did a const generic with default 0xE0 this would not be a breaking API change

[xJonathanLEI]

Weird. I changed the struct definition to:

pub struct APDUCommand<const CLA: u8 = 0xE0> {
  ...
}

but it still gives me an compilation error when I omit the generic param:

error[E0282]: type annotations needed for `common::APDUCommand<_>`
   --> crates/ledger/src/common.rs:314:13
    |
314 |         let command = APDUCommand {
    |             ^^^^^^^
    |
help: consider giving `command` an explicit type, where the value of const parameter `CLA` is specified
    |
314 |         let command: common::APDUCommand<CLA> = APDUCommand {
    |                    ++++++++++++++++++++++++++

I've never worked with default const generic values before. It looks like the compiler accepts the syntax but somehow wouldn't allow omitting it when instantiating? Am I missing anything here? Sorry if this is obvious.

[xJonathanLEI]

Oh I figured out. Don't know how I missed this Stack Overflow answer in the first attempt.

So the solution is to change

        let command = APDUCommand {

into

        let command: APDUCommand = APDUCommand {

and then it magically works. It looks like the default only applies when you name the type, not when you construct it.

But apparently this would mean that using const generic with a default value would still be a breaking change, as downstream code that rely on type inference would no longer compile.

[prestwich]

Would it be worthwhile to add a few type aliases ?

type BitcoinAPDUCommand = APDUCommand<0xE1>;
type StarknetAPDUCommand = APDUCommand<0x5A>;

[xJonathanLEI]

Yeah that would probably be ideal. Posting the above just to say we'll end up breaking users whether we do it field-based or const-generic-based.

I'm making the change towards const generics and fixing the CI now.

[xJonathanLEI]

Hmmm I just realized an issue. Since APDUCommand is used as a field in APDUExchange, which is in turn used in LedgerTask and LedgerHandle, making APDUCommand generic would mean that both APDUExchange, LedgerTask, and LedgerHandle become generic too. Ledger will also become generic since it wraps LedgerHandle.

For APDUExchange and LedgerTask it's probably fine as they can only be instantiated from within the library.

But for LedgerHandle... not so much. When you call ::init() the compiler cannot infer the const generic value. Even if we use a default value, as shown above, it only works when the type is explicitly specified on the left hand side. We could add aliases for it too like BitcoinLedgerHandle and StarknetLedgerHandle but I kinda feel like overengineering.

It's not just structs. The LedgerAsync trait will also need to be updated to make exchange() generic, along with all the implementations. This would mean if any downstream app/lib implements LedgerAsync they're further broken too... for pretty much no good reason.

What do you think? Should we pollute all these types with generics or go back to adding a new cla field? Adding const generic also has the downside of making a LedgerHandle instance only capable of sending a fixed CLA prefix. Not that I can think of use cases when you'd want to change CLA on the fly though..

Overall I just feel like the added complexity isn't worth it when we can just add a field. The breakage from adding a field is actually smaller by comparison.

Just my 2 cents though. Apparently if you still prefer the const generic way I will proceed. Thanks!

[prestwich]

mmm fair point. let's keep it as a prop and publish a breaking change

[xJonathanLEI]

Updated with the clippy and wasm fix.

[xJonathanLEI]

@prestwich Yo mind taking a look? Using this in a downstream crate but cannot publish to crates.io until this is released. Thanks for your time!

[prestwich]

oh sorry i was sure I had merged this. I'll run a release this morning. This is a breaking change so we're going to 0.12.0