review fixes

src/signTx.c

@@ -19,6 +19,22 @@ enum {
 	SIGN_TX_INCLUDED_YES = 2
 };
 
+static bool parseSignTxIncluded(uint8_t value)
+{
+	switch (value) {
+	case SIGN_TX_INCLUDED_YES:
+		return true;
+
+	case SIGN_TX_INCLUDED_NO:
+		return false;
+
+	default:
+		THROW(ERR_INVALID_DATA);
+	}
+}
+
+// TODO remove the following enum in pool operator app if it is not needed
+
 enum {
 	SIGN_TX_POOL_REGISTRATION_NO = 3,
 	SIGN_TX_POOL_REGISTRATION_YES = 4
@@ -323,44 +339,9 @@ static void signTx_handleInitAPDU(uint8_t p2, uint8_t* wireDataBuffer, size_t wi
 		ctx->commonTxData.protocolMagic = u4be_read(wireHeader->protocolMagic);
 		TRACE("protocol magic %d", ctx->commonTxData.protocolMagic);
 
-		switch (wireHeader->includeTtl) {
-		case SIGN_TX_INCLUDED_YES:
-			ctx->includeTtl = true;
-			break;
-
-		case SIGN_TX_INCLUDED_NO:
-			ctx->includeTtl = false;
-			break;
-
-		default:
-			THROW(ERR_INVALID_DATA);
-		}
-
-		switch (wireHeader->includeMetadata) {
-		case SIGN_TX_INCLUDED_YES:
-			ctx->includeMetadata = true;
-			break;
-
-		case SIGN_TX_INCLUDED_NO:
-			ctx->includeMetadata = false;
-			break;
-
-		default:
-			THROW(ERR_INVALID_DATA);
-		}
-
-		switch (wireHeader->includeValidityIntervalStart) {
-		case SIGN_TX_INCLUDED_YES:
-			ctx->includeValidityIntervalStart = true;
-			break;
-
-		case SIGN_TX_INCLUDED_NO:
-			ctx->includeValidityIntervalStart = false;
-			break;
-
-		default:
-			THROW(ERR_INVALID_DATA);
-		}
+		ctx->includeTtl = parseSignTxIncluded(wireHeader->includeTtl);
+		ctx->includeMetadata = parseSignTxIncluded(wireHeader->includeMetadata);
+		ctx->includeValidityIntervalStart = parseSignTxIncluded(wireHeader->includeValidityIntervalStart);
 
 		switch (wireHeader->isSigningPoolRegistrationAsOwner) {
 		case SIGN_TX_POOL_REGISTRATION_YES:
@@ -593,7 +574,7 @@ static void signTx_handleFee_ui_runStep()
 	UI_STEP_BEGIN(ctx->ui_step);
 
 	UI_STEP(HANDLE_FEE_STEP_DISPLAY) {
-		ui_displayAmountScreen("Transaction fee", ctx->stageData.fee, this_fn);
+		ui_displayAdaAmountScreen("Transaction fee", ctx->stageData.fee, this_fn);
 	}
 	UI_STEP(HANDLE_FEE_STEP_RESPOND) {
 		respondSuccessEmptyMsg();
@@ -663,12 +644,10 @@ static void signTx_handleTtl_ui_runStep()
 	UI_STEP_BEGIN(ctx->ui_step);
 
 	UI_STEP(HANDLE_TTL_STEP_DISPLAY) {
-		char ttlString[50];
-		// TODO just display it as a plain uint64, the same as validity interval start
-		str_formatTtl(ctx->stageData.ttl, ttlString, SIZEOF(ttlString));
-		ui_displayPaginatedText(
+		ui_displayValidityBoundaryScreen(
 		        "Transaction TTL",
-		        ttlString,
+		        ctx->stageData.ttl,
+		        ctx->commonTxData.networkId, ctx->commonTxData.protocolMagic,
 		        this_fn
 		);
 	}
@@ -1004,7 +983,7 @@ static void signTx_handleWithdrawal_ui_runStep()
 	UI_STEP_BEGIN(ctx->ui_step);
 
 	UI_STEP(HANDLE_WITHDRAWAL_STEP_DISPLAY) {
-		ui_displayAmountScreen("Withdrawing rewards", ctx->stageData.withdrawal.amount, this_fn);
+		ui_displayAdaAmountScreen("Withdrawing rewards", ctx->stageData.withdrawal.amount, this_fn);
 	}
 	UI_STEP(HANDLE_WITHDRAWAL_STEP_RESPOND) {
 		respondSuccessEmptyMsg();
@@ -1149,7 +1128,7 @@ static void signTx_handleMetadataAPDU(uint8_t p2, uint8_t* wireDataBuffer, size_
 		TRACE_BUFFER(wireDataBuffer, wireDataSize);
 
 		VALIDATE(wireDataSize == METADATA_HASH_LENGTH, ERR_INVALID_DATA);
-		STATIC_ASSERT(SIZEOF(ctx->stageData.metadata.metadataHash) == METADATA_HASH_LENGTH, "inconsistent metadata hash length");
+		STATIC_ASSERT(SIZEOF(ctx->stageData.metadata.metadataHash) == METADATA_HASH_LENGTH, "wrong metadata hash length");
 		os_memmove(ctx->stageData.metadata.metadataHash, wireDataBuffer, SIZEOF(ctx->stageData.metadata.metadataHash));
 		ctx->metadataReceived = true;
 	}
@@ -1186,9 +1165,9 @@ static void signTx_handleMetadataAPDU(uint8_t p2, uint8_t* wireDataBuffer, size_
 // ============================== VALIDITY INTERVAL START ==============================
 
 enum {
-	HANDLE_VALIDITY_INTERVAL_STEP_DISPLAY = 900,
-	HANDLE_VALIDITY_INTERVAL_STEP_RESPOND,
-	HANDLE_VALIDITY_INTERVAL_STEP_INVALID,
+	HANDLE_VALIDITY_INTERVAL_START_STEP_DISPLAY = 900,
+	HANDLE_VALIDITY_INTERVAL_START_STEP_RESPOND,
+	HANDLE_VALIDITY_INTERVAL_START_STEP_INVALID,
 };
 
 static void signTx_handleValidityInterval_ui_runStep()
@@ -1198,21 +1177,22 @@ static void signTx_handleValidityInterval_ui_runStep()
 
 	UI_STEP_BEGIN(ctx->ui_step);
 
-	UI_STEP(HANDLE_VALIDITY_INTERVAL_STEP_DISPLAY) {
-		ui_displayUint64Screen(
+	UI_STEP(HANDLE_VALIDITY_INTERVAL_START_STEP_DISPLAY) {
+		ui_displayValidityBoundaryScreen(
 		        "Validity interval start",
 		        ctx->stageData.validityIntervalStart,
+		        ctx->commonTxData.networkId, ctx->commonTxData.protocolMagic,
 		        this_fn
 		);
 	}
-	UI_STEP(HANDLE_VALIDITY_INTERVAL_STEP_RESPOND) {
+	UI_STEP(HANDLE_VALIDITY_INTERVAL_START_STEP_RESPOND) {
 		respondSuccessEmptyMsg();
 		advanceStage();
 	}
-	UI_STEP_END(HANDLE_VALIDITY_INTERVAL_STEP_INVALID);
+	UI_STEP_END(HANDLE_VALIDITY_INTERVAL_START_STEP_INVALID);
 }
 
-static void signTx_handleValidityIntervalAPDU(uint8_t p2, uint8_t* wireDataBuffer, size_t wireDataSize)
+static void signTx_handleValidityIntervalStartAPDU(uint8_t p2, uint8_t* wireDataBuffer, size_t wireDataSize)
 {
 	{
 		// sanity checks
@@ -1240,8 +1220,8 @@ static void signTx_handleValidityIntervalAPDU(uint8_t p2, uint8_t* wireDataBuffe
 		// select UI step
 		switch (policy) {
 #	define  CASE(POLICY, UI_STEP) case POLICY: {ctx->ui_step=UI_STEP; break;}
-			CASE(POLICY_SHOW_BEFORE_RESPONSE, HANDLE_VALIDITY_INTERVAL_STEP_DISPLAY);
-			CASE(POLICY_ALLOW_WITHOUT_PROMPT, HANDLE_VALIDITY_INTERVAL_STEP_RESPOND);
+			CASE(POLICY_SHOW_BEFORE_RESPONSE, HANDLE_VALIDITY_INTERVAL_START_STEP_DISPLAY);
+			CASE(POLICY_ALLOW_WITHOUT_PROMPT, HANDLE_VALIDITY_INTERVAL_START_STEP_RESPOND);
 #	undef   CASE
 		default:
 			THROW(ERR_NOT_IMPLEMENTED);
@@ -1465,7 +1445,7 @@ static subhandler_fn_t* lookup_subhandler(uint8_t p1)
 		CASE(0x06, signTx_handleCertificateAPDU);
 		CASE(0x07, signTx_handleWithdrawalAPDU);
 		CASE(0x08, signTx_handleMetadataAPDU);
-		CASE(0x09, signTx_handleValidityIntervalAPDU);
+		CASE(0x09, signTx_handleValidityIntervalStartAPDU);
 		CASE(0x0a, signTx_handleConfirmAPDU);
 		CASE(0x0f, signTx_handleWitnessAPDU);
 		DEFAULT(NULL)

src/signTx.h

@@ -83,7 +83,6 @@ typedef struct {
 	uint16_t currentWitness;
 
 // TODO add js tests for missing ttl and inclusion of validity interval start
-// TODO display ttl with pershing routine, also validity interval start, but only on mainnet?
 	bool feeReceived;
 	bool ttlReceived;
 	bool metadataReceived;

src/signTxOutput.c

@@ -72,13 +72,15 @@ static inline void advanceState()
 
 		// we are going to receive token amounts for this group
 		ASSERT(subctx->numTokenAmounts > 0);
+		ASSERT(subctx->currentTokenAmount == 0);
 
 		subctx->state = STATE_OUTPUT_TOKEN_AMOUNT;
 		break;
 
 	case STATE_OUTPUT_TOKEN_AMOUNT:
 		// we are done with the current token group
 		ASSERT(subctx->currentTokenAmount == subctx->numTokenAmounts);
+		subctx->currentTokenAmount = 0;
 		ASSERT(subctx->currentTokenGroup < subctx->numTokenGroups);
 		subctx->currentTokenGroup++;
 
@@ -129,7 +131,7 @@ static void signTx_handleOutput_address_ui_runStep()
 		);
 	}
 	UI_STEP(HANDLE_OUTPUT_ADDRESS_BYTES_STEP_DISPLAY_ADA_AMOUNT) {
-		ui_displayAmountScreen("Send", subctx->stateData.output.adaAmount, this_fn);
+		ui_displayAdaAmountScreen("Send", subctx->stateData.output.adaAmount, this_fn);
 	}
 	UI_STEP(HANDLE_OUTPUT_ADDRESS_BYTES_STEP_RESPOND) {
 		respondSuccessEmptyMsg();
@@ -204,7 +206,7 @@ static void signTx_handleOutput_addressParams_ui_runStep()
 		ui_displayStakingInfoScreen(&subctx->stateData.output.params, this_fn);
 	}
 	UI_STEP(HANDLE_OUTPUT_ADDRESS_PARAMS_STEP_DISPLAY_AMOUNT) {
-		ui_displayAmountScreen("Send", subctx->stateData.output.adaAmount, this_fn);
+		ui_displayAdaAmountScreen("Send", subctx->stateData.output.adaAmount, this_fn);
 	}
 	UI_STEP(HANDLE_OUTPUT_ADDRESS_PARAMS_STEP_RESPOND) {
 		respondSuccessEmptyMsg();
@@ -359,7 +361,7 @@ static void signTxOutput_handleTokenGroup_ui_runStep()
 	UI_STEP_BEGIN(subctx->ui_step);
 
 	UI_STEP(HANDLE_TOKEN_GROUP_STEP_DISPLAY) {
-		STATIC_ASSERT(SIZEOF(subctx->stateData.tokenGroup.policyId) == MINTING_POLICY_ID_SIZE, "inconsistent minting policy id size");
+		STATIC_ASSERT(SIZEOF(subctx->stateData.tokenGroup.policyId) == MINTING_POLICY_ID_SIZE, "wrong minting policy id size");
 
 		ui_displayHexBufferScreen(
 		        "Token policy id",
@@ -391,7 +393,7 @@ static void signTxOutput_handleTokenGroupAPDU(uint8_t* wireDataBuffer, size_t wi
 		read_view_t view = make_read_view(wireDataBuffer, wireDataBuffer + wireDataSize);
 
 		VALIDATE(view_remainingSize(&view) >= MINTING_POLICY_ID_SIZE, ERR_INVALID_DATA);
-		STATIC_ASSERT(SIZEOF(tokenGroup->policyId) == MINTING_POLICY_ID_SIZE, "inconsistent policy id size");
+		STATIC_ASSERT(SIZEOF(tokenGroup->policyId) == MINTING_POLICY_ID_SIZE, "wrong policy id size");
 		view_memmove(tokenGroup->policyId, &view, MINTING_POLICY_ID_SIZE);
 
 		VALIDATE(view_remainingSize(&view) == 4, ERR_INVALID_DATA);
@@ -449,26 +451,7 @@ static void signTxOutput_handleTokenAmount_ui_runStep()
 	UI_STEP_BEGIN(subctx->ui_step);
 
 	UI_STEP(HANDLE_TOKEN_AMOUNT_STEP_DISPLAY_NAME) {
-		token_amount_t* tokenAmount = &subctx->stateData.tokenAmount;
-		if (str_isTextPrintable(tokenAmount->assetName, tokenAmount->assetNameSize)) {
-			char name[ASSET_NAME_SIZE_MAX + 1];
-			ASSERT(tokenAmount->assetNameSize + 1 <= SIZEOF(name));
-			for (size_t i = 0; i < tokenAmount->assetNameSize; i++)
-				name[i] = tokenAmount->assetName[i];
-			name[tokenAmount->assetNameSize] = '\0';
-
-			ui_displayPaginatedText(
-			        "Token name",
-			        name,
-			        this_fn
-			);
-		} else {
-			ui_displayHexBufferScreen(
-			        "Token name",
-			        subctx->stateData.tokenAmount.assetName, subctx->stateData.tokenAmount.assetNameSize,
-			        this_fn
-			);
-		}
+		ui_displayTokenNameScreen(&subctx->stateData.tokenAmount, this_fn);
 	}
 	UI_STEP(HANDLE_TOKEN_AMOUNT_STEP_DISPLAY_AMOUNT) {
 		ui_displayUint64Screen(
@@ -516,7 +499,6 @@ static void signTxOutput_handleTokenAmountAPDU(uint8_t* wireDataBuffer, size_t w
 		VALIDATE(view_remainingSize(&view) == 8, ERR_INVALID_DATA);
 		tokenAmount->amount = parse_u8be(&view);
 		TRACE_UINT64(tokenAmount->amount);
-		// TODO validate something?
 	}
 
 	security_policy_t policy = policyForSignTxOutputTokenAmount(&subctx->stateData.tokenAmount);

src/signTxPoolRegistration.c

@@ -135,14 +135,14 @@ static void handlePoolParams_ui_runStep()
 		);
 	}
 	UI_STEP(HANDLE_POOLPARAMS_STEP_DISPLAY_PLEDGE) {
-		ui_displayAmountScreen(
+		ui_displayAdaAmountScreen(
 		        "Pledge",
 		        subctx->poolParams.pledge,
 		        this_fn
 		);
 	}
 	UI_STEP(HANDLE_POOLPARAMS_STEP_DISPLAY_COST) {
-		ui_displayAmountScreen(
+		ui_displayAdaAmountScreen(
 		        "Cost",
 		        subctx->poolParams.cost,
 		        this_fn

src/textUtils.c

@@ -128,7 +128,7 @@ static struct {
 	{0, 0, 21600}
 };
 
-size_t str_formatTtl(uint64_t ttl, char* out, size_t outSize)
+size_t str_formatValidityBoundary(uint64_t ttl, char* out, size_t outSize)
 {
 	ASSERT(outSize < BUFFER_SIZE_PARANOIA);
 
@@ -182,14 +182,14 @@ void str_validateTextBuffer(const uint8_t* text, size_t textSize)
 	}
 }
 
-// check if it is printable ASCII between 33 and 126
-bool str_isTextPrintable(const uint8_t* text, size_t textSize)
+// check if a non-null-terminated buffer contains printable ASCII between 33 and 126 (inclusive)
+bool str_isAsciiPrintableBuffer(const uint8_t* buffer, size_t bufferSize)
 {
-	ASSERT(textSize < BUFFER_SIZE_PARANOIA);
+	ASSERT(bufferSize < BUFFER_SIZE_PARANOIA);
 
-	for (size_t i = 0; i < textSize; i++) {
-		if (text[i] > 126) return false;
-		if (text[i] <  33) return false;
+	for (size_t i = 0; i < bufferSize; i++) {
+		if (buffer[i] > 126) return false;
+		if (buffer[i] <  33) return false;
 	}
 
 	return true;

src/textUtils.h

@@ -27,12 +27,12 @@ void str_traceUint64(uint64_t number);
 #define TRACE_UINT64(NUMBER)
 #endif
 
-size_t str_formatTtl(uint64_t ttl, char* out, size_t outSize);
+size_t str_formatValidityBoundary(uint64_t ttl, char* out, size_t outSize);
 
 size_t str_formatMetadata(const uint8_t* metadataHash, size_t metadataHashSize, char* out, size_t outSize);
 
 void str_validateTextBuffer(const uint8_t* text, size_t textSize);
-bool str_isTextPrintable(const uint8_t* text, size_t textSize);
+bool str_isAsciiPrintableBuffer(const uint8_t* text, size_t textSize);
 
 
 #ifdef DEVEL

src/textUtils_test.c

@@ -51,15 +51,15 @@ void testcase_formatTtl(
 
 	{
 		char tmp[30];
-		size_t len = str_formatTtl(ttl, tmp, SIZEOF(tmp));
+		size_t len = str_formatValidityBoundary(ttl, tmp, SIZEOF(tmp));
 		EXPECT_EQ(len, strlen(expected));
 		EXPECT_EQ(strcmp(tmp, expected), 0);
 	}
 
 	{
 		// check for buffer overflows
 		char tmp[30];
-		EXPECT_THROWS(str_formatTtl(ttl, tmp, strlen(expected)), ERR_ASSERT);
+		EXPECT_THROWS(str_formatValidityBoundary(ttl, tmp, strlen(expected)), ERR_ASSERT);
 	}
 }
 

src/txHashBuilder.c

@@ -82,7 +82,7 @@ void txHashBuilder_init(
 		builder->includeValidityIntervalStart = includeValidityIntervalStart;
 		if (includeValidityIntervalStart) numItems++;
 
-		ASSERT(3 <= numItems && numItems <= 8);
+		ASSERT((3 <= numItems) && (numItems <= 8));
 
 		TRACE("Serializing tx body with %u items", numItems);
 		BUILDER_APPEND_CBOR(CBOR_TYPE_MAP, numItems);