Token2022 (extensions) support #9

0xMMBD · 2024-02-22T13:04:03Z

Add support for Token2022 instructions(extensions).
Added:

token2022 program id
ProgramIdToken2022 enum value
switch case to handle token2022 in message and transaction_printer
extension names to the token instruction tag enum
unit tests

Fix ci warnings

…xample

workflows/sonarcloud.yml: Update workflow following app-bitcoin-new example

[auto] Update Screenshots

Added: * token2022 program id * ProgramIdToken2022 enum value * switch case to handle token2022 in message and transaction_printer * extension names to the token instruction tag enum

sponomarev

Hey there! I performed a functional test on a real hardware and found that none of the basic operations like transfer, burn, mint, etc. are clear-signed. I faced with an issue of unrecognized format that requires blind signing. Please, see my comment where I assume the logic is broken.

sponomarev · 2024-03-14T17:38:57Z

libsol/message.c

@@ -99,6 +100,8 @@ int process_message_body(const uint8_t* message_body,
            case ProgramIdUnknown:
                display_instruction_info[display_instruction_count++] = info;
                break;
+            //Currently we don't display these instructions
+            case ProgramIdToken2022:


At the moment it seems that the bug is here. We skip the logic where we count instructions and setting instruction info which leads us to unrecognized format handling upper in handle_sign_message_parse_message since process_message_body returns 1.

Hello, thanks for the tests and feedback @sponomarev. It is interesting that "that none of the basic operations" does not work, we've tested it on the real hardware too (NanoX and NanoSP) and it looked fine.
I'll double check that this branch contains the correct code (as it was separated from the other feature recently), but I'm quite sure about that.

In addition to that, are you able to share the Transaction that you used to perform tests? It would be great to reproduce the problem with the same data that you have used. After that we will handle this problem asap.

Sure. Here is an example https://explorer.solana.com/tx/2DRngy89okvFT9gekHKQv5uAy43FVHKndf7qJvm3Q7JbRcE62PKwdHBc1dWWWsK4hW1uTCqNN9WovfXMUNUcKbWC?cluster=devnet. This transaction contains only one instruction from token2022 program. In that case, if we closely take a look at these lines, display_instruction_count won't be incremented and end up as zero, that's passed down to print_transaction and print_transaction_nonce_processed.

You can recreate this transaction easily with spl-token cli.

Thank you, yes, you are right, we will handle this and come back soon

Remove separate Token2022 program id.

0xMMBD · 2024-03-18T21:59:12Z

@sponomarev Hello, I've made some changes. Hopefully I didn't miss anything :)
I've checked transactions with multiple instructions for old spl token as well as for the Token2022 and the same with simple examples like transfer. On my end it looked fine.

In my opinion the best way to handle this was to unify both token types as the same programId. Otherwise handling transaction_printers was getting a little tricky.

Let me know it there is anything else to change or if you find any other problems.

sponomarev

Hey @0xMMBD! It seems that the original issue is resolved. I was able to perform basic token operations for both Token2022 and the original Token program. I've also tried to play with extensions, but nothing was successful. It seems that something goes wrong on that side. Here is my test scenarios report:

✅ Token creation without extensions
✅ Token account creation
✅ Token transfer
✅ Mint token
✅ Burn token

🔴 Token creation with transfer fee extension
🔴 Token creation with confidential transfers extension
🔴 Token creation with default account state extension
🔴 Token account creation with immutable owner extension
🔴 Token account: enable/disable required memo transfers

My raw assumption is that the issue is happening somewhere in print_transaction_nonce_processed where we can mess instruction amount since extensions are enabled with a separate instruction and enable/disable required memo transfers is a separate instruction as well.

0xMMBD · 2024-03-21T09:32:15Z

The extensions are not supported currently. The main purpose of this PR was to properly handle basic operations on token when using Token2022 program. We wanted to make it backward compatible with existing features without the need of blind signing. So current extensions are recognized and are added in the code but we dont have any special logic for them
This scope was discussed with @jnwng

tiago18c

Need clarification on the transfer decoding

tiago18c · 2024-03-21T18:51:22Z

libsol/instruction.c

@@ -18,6 +19,8 @@ enum ProgramId instruction_program_id(const Instruction* instruction, const Mess
        return ProgramIdVote;
    } else if (memcmp(program_id, &spl_token_program_id, PUBKEY_SIZE) == 0) {
        return ProgramIdSplToken;
+    } else if(memcmp(program_id, &spl_token2022_program_id, PUBKEY_SIZE) == 0) {
+        return ProgramIdSplToken;//Treat the Token2022 exactly the same as the SplToken


Is the decoding resulting from this used in the user signing loop?
If so this may cause issues, as the user may be unaware that he is signing transactions using the token22 program that may contain transfer fee or transfer hooks with unintended consequences

The main reason behind this is the code thats prints transaction details. If I detect token2022 as something different then it gets more messy to handle basic token instruction like mint, burn etc. At this point, if we use for example TransferFeeExtension we just ignore it - we don't display any additional screens.

What we can do is to throw error(instruction not supported) when we get any extension instruction. This way there will be no chance to sign something that you are not able to verify visually on the screen. Otherwise we would need to redesign this function https://github.com/blockydevs/app-solana/blob/develop/libsol/transaction_printers.c#L535 https://github.com/blockydevs/app-solana/blob/develop/libsol/transaction_printers.c#L535 because it is not able to handle anything with more than 3 instructions and we dont know what extensions someone will send and in what order

So, like main idea in this PR is to allow transactions that use the token2022 program id to be able to perform basic instructions that are also compatible with the old token

There is a slight mix up between extension setup and the extension usage.

The TransferFeeExtension instruction is only called to setup the extension, so not supporting it here makes total sense. However, if its setup, any calls to Transfer are going to need extra accounts to correctly execute the transfer with the fee.

The way the decoding is currently setup, any Token22 mint with extensions such as Transfer Fees or Transfer Hooks enabled, are going to execute completely blind to the decoding operation due to the Transfer instruction being ABI compatible between both programs (extra accounts needed by extensions are appended at the end, everything else maintains the same binary structure).

At the very least, during decoding, the user should be aware that its signing a Transfer for a mint with such extensions enabled (fees can go all the way up to 100% and can change over time, on the other end of the spectrum transfer hooks can enable bad actors to effectively wipe accounts).

This simple approach seems reasonable at that stage of token2022 support.

Its much better.

However, if I understood this correctly, the warning message only pops up if there are Extensions related instructions on the TX right? For that scenario its the right behaviour.

However, if there is only a TransferChecked instruction in the transaction and it is of a token mint with possible damaging extensions enabled (high transfer fees, draining transfer hook), the user will think he's signing a token transfer from the old Token Program without any side effects as none of that is present in the transaction.

IMO there needs to be another warning on any T22 TransferChecked like:
"Token Extensions" "Cannot verify mint extensions"
"Verify mint issuer before signing"
(not sure of character limits, but something along these lines)

Yes, any extension thats coming from T22 generates this warning, the idea behind this PR was to allow users to use the old backward compatible features.
Due to the current code design we are a little bit limited in terms of detecting any complex cases with different instructions. Transaction printers contains hardcoded switch case that does not allow a lot of flexibility unfortunately.

Can you specify what exact cases should trigger that additional warning? Is it for transactions that only contains single TransferChecked instruction (https://solana-labs.github.io/solana-program-library/token/js/functions/createTransferCheckedInstruction.html) or are they any other cases?

And there is also another issue. On the older nanos devices we are really close to the memory limit, so adding long strings in to the binary may cause problems too, some of them appear only in specific features so maybe we just reuse the same warning text?

The additional warning should be triggered on TransferChecked when programId == token22, regardless of how many instructions are present in the transaction.

I'd say you can recycle the title part of the warnings.

Title: "Extension warning"
Messages: "Can't check mint extensions" -> "Check explorer"

Is the memory available enough for those? If not we can rework these warnings to further reuse strings.

@tiago18c ok, I've added additional warning screens and i've also merged latest develop changes to verify that everything is still working as planned.
I did few tests on the nanos model and it looks that we are still within memory limits

* Added support for compute budget instructions: * request heap frame * set compute unit price * request units * set compute unit limit * added unit tests Co-authored-by: Piotr Swierzy <[email protected]>

* Added split-stake instruction support for: * SystemTransfer + SystemAllocate + SystemAssign + StakeSplit * SystemTransfer + SystemAllocateWithSeed + StakeSplit Co-authored-by: Maciej Nabiałek <[email protected]>

[auto] Update Screenshots

Release v1.4.3

… is used

tiago18c

lgtm

0xMMBD · 2024-05-24T13:45:39Z

Hello @tiago18c can you please summarize what are the next steps? Is there anything else from our side that should be done?

Xavier Chapron and others added 12 commits December 14, 2023 10:46

src: Improve crypto code using SDK crypto_helpers and LEDGER_ASSERT

0b73d10

src: Cleanup unused parameter from BLE_power()

ad37ff9

src: Drop deprectaed check_api_level()

8198345

Merge pull request LedgerHQ#82 from LedgerHQ/xch/fix-ci-warnings

d4098be

Fix ci warnings

workflows/sonarcloud.yml: Update workflow following app-bitcoin-new e…

cfac20e

…xample

Merge pull request LedgerHQ#83 from LedgerHQ/xch/sonarcloud

9f5b1cd

workflows/sonarcloud.yml: Update workflow following app-bitcoin-new example

[auto] Update screenshot

71d0904

Merge pull request LedgerHQ#84 from LedgerHQ/auto-update-screenshots

2d1f62f

[auto] Update Screenshots

[auto] Update screenshot

33e3cc8

Merge pull request LedgerHQ#86 from LedgerHQ/auto-update-screenshots

d769b7a

[auto] Update Screenshots

Add support for Token2022 instructions(extensions).

93ae038

Added: * token2022 program id * ProgramIdToken2022 enum value * switch case to handle token2022 in message and transaction_printer * extension names to the token instruction tag enum

Added Token2022 unit tests

527df19

0xMMBD self-assigned this Feb 22, 2024

0xMMBD changed the base branch from develop to compute-budget-rf-v2 February 22, 2024 13:05

[auto] Update screenshot

2d4f805

sponomarev reviewed Mar 14, 2024

View reviewed changes

0xMMBD requested a review from sponomarev March 18, 2024 21:52

Fix invalid handling of the Token2022 instructions.

5d6fdac

Remove separate Token2022 program id.

0xMMBD force-pushed the pr/token2022-support-v3 branch from 26f9042 to 5d6fdac Compare March 18, 2024 21:55

sponomarev reviewed Mar 20, 2024

View reviewed changes

tiago18c reviewed Mar 21, 2024

View reviewed changes

0xMMBD and others added 2 commits March 29, 2024 16:57

Add compute budget support (#7)

2118779

* Added support for compute budget instructions: * request heap frame * set compute unit price * request units * set compute unit limit * added unit tests Co-authored-by: Piotr Swierzy <[email protected]>

Add warning screen for the user when token2022 extension is present

437603d

0xMMBD and others added 4 commits April 8, 2024 01:05

Added split-stake instruction support (#9) (#10)

e73a9cf

* Added split-stake instruction support for: * SystemTransfer + SystemAllocate + SystemAssign + StakeSplit * SystemTransfer + SystemAllocateWithSeed + StakeSplit Co-authored-by: Maciej Nabiałek <[email protected]>

[auto] Update screenshot

3dfa53e

Merge pull request LedgerHQ#87 from LedgerHQ/auto-update-screenshots

5c5e1b6

[auto] Update Screenshots

Release v1.4.3

ce8ca77

fbeutin-ledger and others added 4 commits April 24, 2024 10:19

Merge pull request LedgerHQ#88 from LedgerHQ/fbe/release_v1.4.3

5138865

Release v1.4.3

Merge branch 'LedgerHQ:develop' into develop

2edf8a2

Merge branch 'develop' into pr/token2022-support-v3

a4e6bb2

Add warning to the TransferChecked instruction when Token2022 program…

028cc29

… is used

0xMMBD requested a review from tiago18c May 9, 2024 17:28

tiago18c approved these changes May 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Token2022 (extensions) support #9

Token2022 (extensions) support #9

0xMMBD commented Feb 22, 2024

sponomarev left a comment •

edited

Loading

sponomarev Mar 14, 2024

0xMMBD Mar 15, 2024

sponomarev Mar 15, 2024

0xMMBD Mar 15, 2024

sponomarev Mar 20, 2024

0xMMBD commented Mar 18, 2024

sponomarev left a comment

0xMMBD commented Mar 21, 2024 •

edited

Loading

tiago18c left a comment

tiago18c Mar 21, 2024

0xMMBD Mar 21, 2024

0xMMBD Mar 21, 2024

0xMMBD Mar 21, 2024

tiago18c Mar 21, 2024

sponomarev Apr 10, 2024

tiago18c Apr 15, 2024

0xMMBD Apr 16, 2024

tiago18c Apr 24, 2024

0xMMBD May 9, 2024

tiago18c left a comment

0xMMBD commented May 24, 2024

Token2022 (extensions) support #9

Are you sure you want to change the base?

Token2022 (extensions) support #9

Conversation

0xMMBD commented Feb 22, 2024

sponomarev left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xMMBD commented Mar 18, 2024

sponomarev left a comment

Choose a reason for hiding this comment

0xMMBD commented Mar 21, 2024 • edited Loading

tiago18c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiago18c left a comment

Choose a reason for hiding this comment

0xMMBD commented May 24, 2024

sponomarev left a comment •

edited

Loading

0xMMBD commented Mar 21, 2024 •

edited

Loading