Make Back Button Work #4
Conversation
…nanos, redo test golden run (subtle differences in rendering)
|
Looks like some of the cryptography functions we used are now deprecated but not yet removed which is causing a bunch of CI errors. Will look into the replacements! |
| uint8_t valLen; // length of outVal | ||
| uint8_t outAddr[77]; // most-recently-seen address | ||
| } txn_elem_t; | ||
|
|
||
| // txn_state_t is a helper object for computing the SigHash of a streamed | ||
| // transaction. | ||
| typedef struct { | ||
| uint8_t buf[510]; // holds raw tx bytes; large enough for two 0xFF reads |
There was a problem hiding this comment.
if we're no longer streaming, perhaps we could use a union here? As in, use the same memory to store both buf and elements. The unparsed elements in buf should always take up more space than their parsed equivalent, so we shouldn't be at risk of overwriting unparsed bytes.
There was a problem hiding this comment.
But there can be multiple reads. So if we're the Nano S and we've read in say 10 elements (10*(4 + 24 + 32) = 600 bytes) but we still need to read in more elements then we'd have a problem. Also I think it might be undefined behavior to use a union like that. Or maybe I'm misunderstanding something.
…command due to uninitialized variable elementIndex in the context, make error handling consistent across devices
|
Alright, I think I got it. I am going to test this on a physical Nano S and figure out why the ragger tests aren't running tomorrow. Then this should be good to merge. |
|
I was not able to test on my physical Ledger S because I couldn't get my computer to recognize the device so that I could flash the app despite trying a bunch of different things. But I don't see any reason why it wouldn't work given that speculos is a nearly perfect emulator. |
|
Unless anyone has additional suggestions (which if there are any on the memory saving changes I just made I'd appreciate it) I think this should be OK to merge. Technically you can compile with a slightly higher element limit for the Nano S but the app will crash if you do more than one action. I did not encounter crashes at 22 elements (but I did at 24) so I went with 20 to have some margin of safety. |
|
20 should be fine 👍🏻 |
This PR allows us to use backward and forward navigation in the Stax app. To do that, it required changing the code that streams in the transaction to read the whole transaction in at once. Unfortunately this means we have to store a fixed size buffer of transaction elements. The code currently stores only the elements that we actually need to display.
The limit to how many elements you can store varies by device but when you make too the limit large, you get this kind of error when building:
For most devices (NanoX, NanoSP, Stax) I just settled on 64 maximum elements as 96 and 128 were too large. But the Nano S has a ridiculously small amount of RAM and can only store 6 elements. So I was unsure how to proceed here without causing large amounts of code duplication. I hope we are able to just leave the Nano S build based on the older version of the code as is.