device_ledger: factor the prologue code

[moneromooo-monero]

No description provided.

[stoffu]

Unused?

[stoffu]

Why is it necessary to have prologue and prologue_noopt separately? Can't we have just one prologue as:

int device_ledger::prologue(BYTE type, BYTE extra) {
  reset_buffer();
  this->buffer_send[0] = 0x00;
  this->buffer_send[1] = type;
  this->buffer_send[2] = extra;
  this->buffer_send[3] = 0x00;
  this->buffer_send[4] = 0x00;
  int offset = 5;
  //options
  this->buffer_send[offset++] = 0;
  return offset;
}

void device_ledger::send_simple(BYTE type, BYTE extra) {
  int offset = prologue(type, extra);
  this->buffer_send[4] = offset - 5;
  this->length_send = offset;
  this->exchange();
}

[moneromooo-monero]

Just to allow some more factoring while avoiding a function with (what I think would be) too many parameters.

[stoffu]

But this way of split seems weird to me, because AIUI this line

this->buffer_send[4] = offset - 5;

is supposed to go together with these lines

this->length_send = offset;
this->exchange();

I.e. the content of buffer_send[4] indicates the size of optional argument data starting from buffer_send[5]. In the current patch, the call to prologue_noopt() puts 1 to buffer_send[4], which will then be overwritten by the subsequent line this->buffer_send[4] = offset-5;. The end result is the same, but this patch adds an unnecessary step of buffer_send[4] = 1 which seems weird to me. I also don't really understand what you said above (to allow more factoring and to avoid a function with too many parameters). Can you elaborate?

[moneromooo-monero]

AFAICT, buffer_send[4] is a message length, so it looks right. This step is only unnecessary when the prologue is followed by other data, increasing the message length.
What I meant by a function with more parameters is that there could be one single function which you configure with command byte, extra byte, optional byte, and possibly more. I simply decided not to do it that way because I judged it less clear/too annoying for the caller.

[stoffu]

OK, thanks for the clarification.

[stoffu]

Looks good to me, though approval from @cslashm would be very desirable

[moneromooo-monero]

one more call using send_simple

[moneromooo-monero]

Expand the prologue functions to also factor a couple more variants, per request.

[cslashm]

Except the naming, looks fine for me .

[cslashm]

In order to respect the ISO7816 protocol naming (which is used by PCSC and the NanosApp) I 'd prefere this naming:

int device_ledger::set_command_header(BYTE ins , BYTE p1, BYTE p2)

[cslashm]

In order to respect the ISO7816 protocol naming (which is used by PCSC and the NanosApp) I 'd prefere this naming:

int device_ledger::set_command_header_noopt(BYTE ins , BYTE p1, BYTE p2)