feat: signal bits and authority outputs by r4mmer · Pull Request #67 · HathorNetwork/hathor-ledger-app

r4mmer · 2024-05-17T16:07:27Z

Acceptance criteria

Add signal bits parsing.
Allow authority outputs parsing and confirmation.

r4mmer · 2024-05-20T17:24:10Z

The new screens for a random test token UBQ would be:

These screens would replace the Amount screen when a token is an authority

r4mmer · 2024-05-23T14:12:21Z

Reference for reviewers:

Macros

PRINTF: this macro only prints when the app is built in debug mode, so we can use it as a dev only log.
- During production builds its discarded, not even loading the parameters (in case we log something sensitive)
THROW: Ledger sdk creates a try..catch macro that we use on some methods (and on the main function), this THROW is expected to be inside one.
- I believe it fails compilation if not inside a try..catch

UI Macros are a separate category since they are used to organize the screens and user interaction, it works by building steps (screens) and flows (similar to menus, with multiple screens).

User interaction on a step is interpreted as pressing both buttons at the same time.

UX_STEP_NOCB: Show some information, no user interaction
- Parameters are name, type, args
UX_STEP_CB: Show information with user interaction
- Parameters are name, type, callback, args
UX_FLOW: group of steps to show, if it ends with FLOW_LOOP the last step loops back to the first one.
- This is how we create cohesive menus

src/ui/display.c

pedroferreira1 · 2024-09-27T14:00:43Z

src/transaction/deserialize.c

+}
+
+bool is_mint_authority(uint8_t token_data, uint64_t value) {
+    return is_authority_output(token_data) && value == MINT_AUTHORITY_MASK;


I think this should be && (value & MINT_AUTHORITY_MASK) > 0, shouldn't it?

The same for melt

The problem with this is the Mint+Melt case, where an output is authority for both mint and melt.
This edge case may be complicated for the user so I decided not to allow this case in the Ledger app.

Do you think we should allow this case as a special Mint+Melt output on the UI?

I think it's ok to limit this but I don't see as this complicated, we could show exactly as you wrote: Mint + Melt or Mint and Melt.

If we decide to leave this out, we should just make clear in the design

I’ve reopened this thread to address the issue. I believe we need to correctly implement the logic in these methods, while managing the mint and melt permissions elsewhere. The current approach might be misleading and prone to errors during future updates.

Regarding the minting and melting capabilities, I found the previous discussion a bit confusing. It doesn’t seem possible to mint and melt the same token simultaneously. So, the real question is whether a token authority should have both capabilities. I believe they should.

I forgot to put the reason the discussion was closed, this was meant to be discussed and implemented on another PR.
We can block this PR to solve this issue but this can easily be implemented later depending on the decision.

src/ui/display.c

…od-01 * origin/dev: chore: update linter version chore: update CI (#77) feat: use correct size limit fix: code scanning [auto] Add guideline enforcer [auto] Add manifest [auto]: add PR template

…od-01 * origin/dev: fix: stop confirming outputs before parsing them (#70) chore: changelog updates and utils (#72)

msbrogli · 2024-10-04T16:17:03Z

src/transaction/deserialize.c

+}
+
+bool is_mint_authority(uint8_t token_data, uint64_t value) {
+    return is_authority_output(token_data) && value == MINT_AUTHORITY_MASK;


I’ve reopened this thread to address the issue. I believe we need to correctly implement the logic in these methods, while managing the mint and melt permissions elsewhere. The current approach might be misleading and prone to errors during future updates.

Regarding the minting and melting capabilities, I found the previous discussion a bit confusing. It doesn’t seem possible to mint and melt the same token simultaneously. So, the real question is whether a token authority should have both capabilities. I believe they should.

msbrogli · 2024-10-04T16:17:09Z

src/transaction/deserialize.c

+}
+
+bool is_melt_authority(uint8_t token_data, uint64_t value) {
+    return is_authority_output(token_data) && value == MELT_AUTHORITY_MASK;


msbrogli · 2024-10-04T16:19:03Z

tests/qa.py

    signatures = cmd.sign_tx(tx)


+def test_qa_sign_tx_with_token(cmd):


i can't find where you used this function.

The QA file is not meant to run on the automated tests.

The idea of these tests is to run with a dev so the dev can check that all screens (UI/UX) are working as intended.
While we do not have a QA for the Ledger I like to keep this file updated with new methods and features since I also use this qa.py for simple tests.

msbrogli · 2024-10-04T16:19:38Z

tests/qa.py

    tx = Transaction(0, 1, [token.uid], inputs, outputs)
-    print("QA::sign_tx_with_authority::token:", str(token))
-    print("QA::sign_tx_with_authority::tx:", str(tx))
+    print("QA::sign_tx_with_aauthority::token:", str(token))


msbrogli · 2024-10-04T16:19:44Z

tests/qa.py

-    print("QA::sign_tx_with_authority::token:", str(token))
-    print("QA::sign_tx_with_authority::tx:", str(tx))
+    print("QA::sign_tx_with_aauthority::token:", str(token))
+    print("QA::sign_tx_with_aauthority::tx:", str(tx))


msbrogli · 2024-10-04T16:20:54Z

tests/test_sign_cmd.py

+
+def test_sign_tx_with_authority(cmd, public_key_bytes):
+    token = fake_token()
+    inputs = [


I can't find the mint and melt authorities. Where are they?

I was expecting to find the following tests:

Minting tokens.

Melting tokens.

Destroying mint authority.

Destroying melt authority.

Delegating mint authority.

Delegating melt authority.

Each case with Mint, Melt, and Mint+Melt authorities.

This will also be discussed later but a Mint Transaction is a transaction with:

Mint Authority input

HTR input

Minted Token output

Optional: Mint authority output

Optional: HTR change output

Since inputs are not validated, a transaction with a mint authority output and a valid token is indistinguishable from a Mint transaction, same with Melt authorities.
So testing that we can show and confirm a Mint or Melt output is enough to test a Mint or Melt operation.
The same logic applies with Destroy and Delegate.

msbrogli · 2024-10-04T16:24:59Z

src/ui/display.c

+            } else {
+                // This authority is unknown, so we treat it as invalid
+                PRINTF("[-] Unknown authority received in value %d\n", output.value);
+                explicit_bzero(&G_context, sizeof(G_context));


Should we clear globals here?

This is clearing the global context.
The G_token_symbols can be cleared but it is not a security issue to leave them initialized since only user confirmed tokens enter the G_token_symbols context.

msbrogli · 2024-10-04T16:25:50Z

src/ui/display.c

+        // set g_authority
+        strlcpy(g_authority, symbol, MAX_TOKEN_SYMBOL_LEN + 1);
+        g_authority[symbol_len] = ' ';
+        if (is_mint_authority(output.token_data, output.value)) {


Aren't you showing what action is being performed? I mean, you can have a mint authority with the following actions: (i) minting, (ii) destroying, and (iii) delegating.

msbrogli · 2024-10-04T16:26:27Z

src/ui/display.c

+UX_STEP_NOCB(ux_display_authority_step,
+             bnnn_paging,
+             {
+                 .title = "Authority",


Do we have room for "Token Authority"? Or is it too big?

feat: signal bits and authority outputs

13bc742

r4mmer self-assigned this May 17, 2024

r4mmer added 3 commits May 17, 2024 12:12

chore: linter changes

b99892e

feat: display authority output confirmation

254771d

tests: signal bits on tests

1c16ec7

r4mmer added 5 commits May 20, 2024 13:30

feat: authority outputs should only be mint/melt

bf2e575

tests: custom token tests and debug stmts

296e146

chore: remove unused tx_version

a7ce0d7

chore: change call for parse_output

d9e8adc

chore: linter changes

6776d97

r4mmer requested review from glevco, msbrogli and pedroferreira1 May 20, 2024 20:08

r4mmer commented May 23, 2024

View reviewed changes

src/ui/display.c Show resolved Hide resolved

chore: remove unnecessary comment

9256cb2

r4mmer removed the request for review from glevco June 13, 2024 16:41

r4mmer mentioned this pull request Sep 26, 2024

chore: changelog updates and utils #72

Merged

pedroferreira1 reviewed Sep 27, 2024

View reviewed changes

r4mmer added 2 commits October 2, 2024 13:59

Merge remote-tracking branch 'origin/dev' into feat/ledger-admin-meth…

7b0639d

…od-01 * origin/dev: chore: update linter version chore: update CI (#77) feat: use correct size limit fix: code scanning [auto] Add guideline enforcer [auto] Add manifest [auto]: add PR template

chore: review changes

7de584f

r4mmer requested a review from pedroferreira1 October 2, 2024 19:20

pedroferreira1 approved these changes Oct 3, 2024

View reviewed changes

Merge remote-tracking branch 'origin/dev' into feat/ledger-admin-meth…

7068697

…od-01 * origin/dev: fix: stop confirming outputs before parsing them (#70) chore: changelog updates and utils (#72)

r4mmer force-pushed the feat/ledger-admin-method-01 branch from dd6f3ca to 7068697 Compare October 3, 2024 16:28

msbrogli requested changes Oct 4, 2024

View reviewed changes

		signatures = cmd.sign_tx(tx)


		def test_qa_sign_tx_with_token(cmd):

Conversation

r4mmer commented May 17, 2024

Acceptance criteria

Uh oh!

r4mmer commented May 20, 2024

Uh oh!

r4mmer commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Macros

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

r4mmer commented May 23, 2024 •

edited

Loading