forked from LedgerHQ/app-solana
-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Token2022 (extensions) support #9
Open
0xMMBD
wants to merge
24
commits into
compute-budget-rf-v2
Choose a base branch
from
pr/token2022-support-v3
base: compute-budget-rf-v2
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
0b73d10
src: Improve crypto code using SDK crypto_helpers and LEDGER_ASSERT
ad37ff9
src: Cleanup unused parameter from BLE_power()
8198345
src: Drop deprectaed check_api_level()
d4098be
Merge pull request #82 from LedgerHQ/xch/fix-ci-warnings
xchapron-ledger cfac20e
workflows/sonarcloud.yml: Update workflow following app-bitcoin-new e…
9f5b1cd
Merge pull request #83 from LedgerHQ/xch/sonarcloud
xchapron-ledger 71d0904
[auto] Update screenshot
2d1f62f
Merge pull request #84 from LedgerHQ/auto-update-screenshots
sgliner-ledger 33e3cc8
[auto] Update screenshot
d769b7a
Merge pull request #86 from LedgerHQ/auto-update-screenshots
sgliner-ledger 93ae038
Add support for Token2022 instructions(extensions).
0xMMBD 527df19
Added Token2022 unit tests
0xMMBD 2d4f805
[auto] Update screenshot
sgliner-ledger 5d6fdac
Fix invalid handling of the Token2022 instructions.
0xMMBD 2118779
Add compute budget support (#7)
0xMMBD 437603d
Add warning screen for the user when token2022 extension is present
0xMMBD e73a9cf
Added split-stake instruction support (#9) (#10)
0xMMBD 3dfa53e
[auto] Update screenshot
5c5e1b6
Merge pull request #87 from LedgerHQ/auto-update-screenshots
sgliner-ledger ce8ca77
Release v1.4.3
fbeutin-ledger 5138865
Merge pull request #88 from LedgerHQ/fbe/release_v1.4.3
fbeutin-ledger 2edf8a2
Merge branch 'LedgerHQ:develop' into develop
0xMMBD a4e6bb2
Merge branch 'develop' into pr/token2022-support-v3
0xMMBD 028cc29
Add warning to the TransferChecked instruction when Token2022 program…
0xMMBD File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
#include "sol/print_config.h" | ||
#include "common_byte_strings.h" | ||
|
||
|
||
const Pubkey spl_token2022_program_id = {{PROGRAM_ID_SPL_TOKEN_2022}}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
#pragma once | ||
|
||
#include "sol/parser.h" | ||
#include "sol/print_config.h" | ||
#include "sol/transaction_summary.h" | ||
#include "spl/token.h" | ||
#include "instruction.h" | ||
|
||
#define SplTokenExtensionKind(b) Token_TokenExtensionInstruction_##b | ||
|
||
extern const Pubkey spl_token2022_program_id; | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 orderThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toTransfer
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This simple approach seems reasonable at that stage of token2022 support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional warning should be triggered on
TransferChecked
whenprogramId == 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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