From 8881471a8e567cf1d76182826d757965c9a48278 Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Thu, 16 Mar 2023 18:16:35 +0100 Subject: [PATCH] EIP-712 fixes - Now displays a Review page before showing the fields - Now asks for confirmation before rejecting - Now does not refresh the screen twice after a signature approval/rejection - Both EIP-712 implementations on par UX-wise - Fixed v0 approve/reject status word mismatch - Unified NBGL UI of both implementations --- src_features/signMessageEIP712/context_712.c | 1 - src_nbgl/ui_712_common.c | 48 +++++++++ src_nbgl/ui_712_common.h | 13 +++ src_nbgl/ui_sign_712.c | 108 +++++++++---------- src_nbgl/ui_sign_712_v0.c | 53 ++++----- 5 files changed, 134 insertions(+), 89 deletions(-) create mode 100644 src_nbgl/ui_712_common.c create mode 100644 src_nbgl/ui_712_common.h diff --git a/src_features/signMessageEIP712/context_712.c b/src_features/signMessageEIP712/context_712.c index 82abd09..57d5be6 100644 --- a/src_features/signMessageEIP712/context_712.c +++ b/src_features/signMessageEIP712/context_712.c @@ -72,7 +72,6 @@ void eip712_context_deinit(void) { mem_reset(); eip712_context = NULL; reset_app_context(); - ui_idle(); } #endif diff --git a/src_nbgl/ui_712_common.c b/src_nbgl/ui_712_common.c new file mode 100644 index 0000000..a1b13e3 --- /dev/null +++ b/src_nbgl/ui_712_common.c @@ -0,0 +1,48 @@ +#include "ui_nbgl.h" +#include "ui_712_common.h" +#include "common_712.h" + +static void (*g_resume_func)(void) = NULL; + +void nbgl_712_review_approve(void) { + ui_712_approve_cb(NULL); +} + +void nbgl_712_review_reject(void) { + ui_712_reject_cb(NULL); +} + +void nbgl_712_confirm_rejection_cb(bool confirm) { + if (confirm) { + nbgl_useCaseStatus("Message signing\ncancelled", false, nbgl_712_review_reject); + } else { + (*g_resume_func)(); + } +} + +void nbgl_712_confirm_rejection(void) { + nbgl_useCaseChoice(&C_warning64px, + "Reject message?", + NULL, + "Yes, reject", + "Go back to message", + nbgl_712_confirm_rejection_cb); +} + +void nbgl_712_review_choice(bool confirm) { + if (confirm) { + nbgl_useCaseStatus("MESSAGE\nSIGNED", true, nbgl_712_review_approve); + } else { + nbgl_712_confirm_rejection(); + } +} + +void nbgl_712_start(void (*resume_func)(void), const char *title) { + g_resume_func = resume_func; + nbgl_useCaseReviewStart(&C_Message_64px, + title, + NULL, + "Reject", + resume_func, + nbgl_712_confirm_rejection); +} diff --git a/src_nbgl/ui_712_common.h b/src_nbgl/ui_712_common.h new file mode 100644 index 0000000..ab8bd4f --- /dev/null +++ b/src_nbgl/ui_712_common.h @@ -0,0 +1,13 @@ +#ifndef UI_712_COMMON_H_ +#define UI_712_COMMON_H_ + +#include + +void nbgl_712_approve(void); +void nbgl_712_reject(void); +void nbgl_712_confirm_rejection_cb(bool confirm); +void nbgl_712_confirm_rejection(void); +void nbgl_712_review_choice(bool confirm); +void nbgl_712_start(void (*resume_func)(void), const char *title); + +#endif // UI_712_COMMON_H_ diff --git a/src_nbgl/ui_sign_712.c b/src_nbgl/ui_sign_712.c index ab40a9e..094fdb6 100644 --- a/src_nbgl/ui_sign_712.c +++ b/src_nbgl/ui_sign_712.c @@ -1,29 +1,16 @@ +#ifdef HAVE_EIP712_FULL_SUPPORT + #include "common_ui.h" #include "ui_nbgl.h" #include "ui_logic.h" #include "common_712.h" #include "nbgl_use_case.h" #include "network.h" +#include "ui_712_common.h" -// 4 pairs of tag/value to display -static nbgl_layoutTagValue_t tlv; +static nbgl_layoutTagValue_t pair; -static void reject_message(void) { - ui_712_reject(NULL); -} - -static void sign_message() { - ui_712_approve(NULL); -} - -static void reviewChoice(bool confirm) { - if (confirm) { - sign_message(); - } else { - reject_message(); - } -} -static bool displaySignPage(uint8_t page, nbgl_pageContent_t *content) { +static bool display_sign_page(uint8_t page, nbgl_pageContent_t *content) { (void) page; content->type = INFO_LONG_PRESS, content->infoLongPress.icon = get_app_icon(true); content->infoLongPress.text = "Sign typed message"; @@ -31,50 +18,63 @@ static bool displaySignPage(uint8_t page, nbgl_pageContent_t *content) { return true; } -static uint32_t stringsIdx = 0; +static bool display_review_page(uint8_t page, nbgl_pageContent_t *content) { + bool ret; + uint16_t len; -static bool displayTransactionPage(uint8_t page, nbgl_pageContent_t *content) { - uint16_t len = 0; - if (stringsIdx < strlen(strings.tmp.tmp)) { - bool reached = nbgl_getTextMaxLenInNbLines(BAGL_FONT_INTER_REGULAR_32px, - strings.tmp.tmp + stringsIdx, - SCREEN_WIDTH - (2 * BORDER_MARGIN), - 9, - &len); - memset(staxSharedBuffer, 0, sizeof(staxSharedBuffer)); - memcpy(staxSharedBuffer, strings.tmp.tmp + stringsIdx, len); - stringsIdx += len; - tlv.item = strings.tmp.tmp2; - tlv.value = staxSharedBuffer; - content->type = TAG_VALUE_LIST; - content->tagValueList.nbPairs = 1; - content->tagValueList.pairs = &tlv; - return true; - } else { - stringsIdx = 0; - switch (ui_712_next_field()) { - case EIP712_NO_MORE_FIELD: - return displaySignPage(page, content); - break; - case EIP712_FIELD_INCOMING: - case EIP712_FIELD_LATER: - default: - break; - } - return false; + switch (page) { + case 0: + // limit the value to one page + nbgl_getTextMaxLenInNbLines(BAGL_FONT_INTER_REGULAR_32px, + strings.tmp.tmp, + SCREEN_WIDTH - (2 * BORDER_MARGIN), + 9, + &len); + strings.tmp.tmp[len] = '\0'; + + pair.item = strings.tmp.tmp2; + pair.value = strings.tmp.tmp; + content->type = TAG_VALUE_LIST; + content->tagValueList.nbPairs = 1; + content->tagValueList.pairs = &pair; + content->tagValueList.wrapping = false; + content->tagValueList.nbMaxLinesForValue = 0; + ret = true; + break; + + case 1: + switch (ui_712_next_field()) { + case EIP712_NO_MORE_FIELD: + return display_sign_page(page, content); + break; + case EIP712_FIELD_INCOMING: + case EIP712_FIELD_LATER: + default: + break; + } + __attribute__((fallthrough)); + + default: + ret = false; + break; } + return ret; } -void ui_712_switch_to_sign(void) { - nbgl_useCaseRegularReview(0, 0, "Reject", NULL, displaySignPage, reviewChoice); +static void handle_display(nbgl_navCallback_t cb) { + nbgl_useCaseRegularReview(0, 0, "Reject", NULL, cb, nbgl_712_review_choice); } void ui_712_start(void) { - stringsIdx = 0; - nbgl_useCaseRegularReview(0, 0, "Reject", NULL, displayTransactionPage, reviewChoice); + nbgl_712_start(&ui_712_switch_to_message, "Review typed message"); } void ui_712_switch_to_message(void) { - stringsIdx = 0; - nbgl_useCaseRegularReview(0, 0, "Reject", NULL, displayTransactionPage, reviewChoice); + handle_display(display_review_page); } + +void ui_712_switch_to_sign(void) { + handle_display(display_sign_page); +} + +#endif // HAVE_EIP712_FULL_SUPPORT diff --git a/src_nbgl/ui_sign_712_v0.c b/src_nbgl/ui_sign_712_v0.c index df9a547..feca3bf 100644 --- a/src_nbgl/ui_sign_712_v0.c +++ b/src_nbgl/ui_sign_712_v0.c @@ -2,41 +2,34 @@ #include "ui_nbgl.h" #include "common_712.h" #include "network.h" +#include "ethUtils.h" +#include "ui_712_common.h" static nbgl_layoutTagValue_t tlv[2]; -static char domain_hash[70]; -static char message_hash[70]; +static void start_review(void); // forward declaration -static void reviewReject(void) { - ui_712_approve_cb(NULL); +static char *format_hash(const uint8_t *hash, char *buffer, size_t buffer_size, size_t offset) { + snprintf(buffer + offset, buffer_size - offset, "0x%.*H", KECCAK256_HASH_BYTESIZE, hash); + return buffer + offset; } -static void confirmTransation(void) { - ui_712_reject_cb(NULL); -} - -static void reviewChoice(bool confirm) { - if (confirm) { - // display a status page and go back to main - nbgl_useCaseStatus("MESSAGE\nSIGNED", true, confirmTransation); - } else { - nbgl_useCaseStatus("Message signing\ncancelled", false, reviewReject); - } -} - -static bool displayTransactionPage(uint8_t page, nbgl_pageContent_t *content) { - snprintf(domain_hash, 70, "0x%.*H", 32, tmpCtx.messageSigningContext712.domainHash); - snprintf(message_hash, 70, "0x%.*H", 32, tmpCtx.messageSigningContext712.messageHash); - +static bool display_review_page(uint8_t page, nbgl_pageContent_t *content) { if (page == 0) { tlv[0].item = "Domain hash"; - tlv[0].value = domain_hash; + tlv[0].value = format_hash(tmpCtx.messageSigningContext712.domainHash, + strings.tmp.tmp, + sizeof(strings.tmp.tmp), + 0); tlv[1].item = "Message hash"; - tlv[1].value = message_hash; + tlv[1].value = format_hash(tmpCtx.messageSigningContext712.messageHash, + strings.tmp.tmp, + sizeof(strings.tmp.tmp), + 70); content->type = TAG_VALUE_LIST; content->tagValueList.nbPairs = 2; + content->tagValueList.nbMaxLinesForValue = 0; content->tagValueList.pairs = (nbgl_layoutTagValue_t *) tlv; } else if (page == 1) { content->type = INFO_LONG_PRESS, content->infoLongPress.icon = get_app_icon(true); @@ -48,19 +41,11 @@ static bool displayTransactionPage(uint8_t page, nbgl_pageContent_t *content) { // valid page so return true return true; } -static void reviewContinue(void) { - nbgl_useCaseRegularReview(0, 2, "Reject", NULL, displayTransactionPage, reviewChoice); -} -static void buildFirstPage(void) { - nbgl_useCaseReviewStart(get_app_chain_icon(), - "Sign typed message", - NULL, - "Reject", - reviewContinue, - reviewReject); +static void start_review(void) { + nbgl_useCaseRegularReview(0, 2, "Reject", NULL, display_review_page, nbgl_712_review_choice); } void ui_sign_712_v0(void) { - buildFirstPage(); + nbgl_712_start(&start_review, "Sign typed message"); }