feat: token admin methods #73
Conversation
|
|
||
| --- | ||
|
|
||
| Currently we only allow P2PKH scripts (and addresses) but NFTs have a data output. |
There was a problem hiding this comment.
Under "Future possibilities" is discussed the difficulties with P2SH, while this design may not comtemplate P2SH support we may add partial or full support in the future.
Partial support would entail showing P2SH addresses on the transaction confirmation.
Full support would entail address generation (basically a P2SH mode on the app) and recognizing P2SH change outputs.
|
|
||
| | Description | P1 | P2 | CData | | ||
| | ------------------------------------------- | ---- | ---- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Send token data<br>for signing transactions | 0x00 | 0x00 | `version (1)` \|\|<br> `uid (32)` \|\|<br>`symbol_len (1)` \|\|<br>`symbol (symbol_len)` \|\|<br>`name_len (1)` \|\|<br>`name (name_len)` \|\|<br>`signature (32)` | |
There was a problem hiding this comment.
This command seems to be very specific. Why not send the transaction bytes with a desired type and let the Ledger app parse and handle it? This way the Ledger app would support more advanced use cases without any change.
There was a problem hiding this comment.
What do you mean? Do you want to send the full transaction in bytes to the ledger, and we create a method to parse it there?
There was a problem hiding this comment.
We send the sighash_all data which is the full transaction, there are a few issues with this.
- The apdu protocol allows us to send up to 255 bytes with each call
- The name and symbol of the token being created sits at the end of the sighash_all
- When parsing the outputs we ask the user for confirmation before asking for the next 255 bytes.
- Since we have about 4k of RAM in the nanoS we cannot hold all metadata and the full
sighash_allwhen running the command.
- Since we have about 4k of RAM in the nanoS we cannot hold all metadata and the full
So when the output with token_data of the created token is parsed, we know the uid but not the name and symbol.
We would not be able to show the user the amount as TST 1.00 since we do not know the symbol.
I discussed a few solutions for this with @pedroferreira1
- Create a completely separate sign_tx method for token creation (too much code duplication, not maintainable)
- Rework all the transaction parsing to be able to get the name and symbol at the end.
- While possible, this would be a very big project since transaction parsing is more than half of the complexity of our app.
- Simply show the token being created amount in a screen with "Created token amount"
- We know this is a create token transaction from the version.
- We know the output is of the token being created because the token_data.
- Very simple implementation and indicative of what the output does.
- Load the name and symbol before sending the transaction, the user will have to confirm the creation of the token before the Ledger even receives the tx.
- Extremely indicative of what the transaction does and can be aborted if the version does not match a create token transaction.
- Better UX, since the flow would be:
- Confirm that you want to create a new token or NFT.
- Confirm the name, symbol and data of the token.
- Confirm the transaction normally
- Since name, symbol and data are already confirmed we can just check that they match and not ask for second confirmation.
- We can show the symbol on the amount screen as with all other outputs.
For the stated reasons I went with the last approach, but we can go with another one if you think it's better.
|
|
||
| ### Mint, Melt, Delegate and Destroy | ||
|
|
||
| Sending a mint, melt, delegate or destroy transactions should use the same protocol as sending normal transactions. |
There was a problem hiding this comment.
How are we going to present which is the token id that is being minted/melted/delegated/destroyed? I feel that we should present the Token Name, Token Symbol and Token UID.
I also think that the wallet should send the TokenCreationTransaction to the Ledger app, so it doesn't have to trust metadata. Then, the Ledger app will calculate the hash, confirm it is the same as the token_id in the new transaction, and, finally, get the token name and token symbol from the TokenCreationTransaction.
There was a problem hiding this comment.
I think this differs from what we have nowadays when sending a custom token. Should we add this same security step to these transactions?
There was a problem hiding this comment.
How are we going to present which is the token id that is being minted/melted/delegated/destroyed? I feel that we should present the Token Name, Token Symbol and Token UID.
Since the Ledger does not have the transaction from the inputs we are somewhat limited here.
Having all spent transactions sent to Ledger is not feasible either since it may not have enough memory for this (nanoS has about 4.5Kb of RAM).
So a mint operation that does not create a new mint authority is indistinguishable (from the outputs only) from a transaction that is sending some tokens, we may add some information to make the UX nicer, but the ledger may not be able to check that mint/melt/destroy operations are being done (delegate operations always have an authority output).
An example of this is that if we do not send authority outputs we can already mint and melt tokens and destroy token authorities with the Ledger device.
I also think that the wallet should send the TokenCreationTransaction to the Ledger app, so it doesn't have to trust metadata. Then, the Ledger app will calculate the hash, confirm it is the same as the token_id in the new transaction, and, finally, get the token name and token symbol from the TokenCreationTransaction.
Currently the user has to manually confirm any tokens that the he wants to send in transactions with the Ledger, this process is called token signing and must be done before we attempt to sign a transaction with the token (the app will block a transaction if a token in the token array was not signed by the user).
We could add the "TokenCreationTransaction hash matching the UID" check to this token signing to make it even safer, but I think we should avoid blindly trusting a token to be sent using the Ledger just because it matches the TokenCreationTransaction.
Also, I think this would merit a design of its own.
|
|
||
| ### Mint, Melt, Delegate and Destroy | ||
|
|
||
| Sending a mint, melt, delegate or destroy transactions should use the same protocol as sending normal transactions. |
There was a problem hiding this comment.
I think this differs from what we have nowadays when sending a custom token. Should we add this same security step to these transactions?
| Delegate is simpler since its actually just sending an authority output. | ||
|
|
||
| Destroy operations usually do not even have authority outputs, so it actually is already supported. | ||
| Granted that the token array will have the token uid of the authority being destroyed so the user still has to sign the token data even with no outputs of the token. |
There was a problem hiding this comment.
How are we going to show to the user that X authority outputs are being destroyed in this transaction?
There was a problem hiding this comment.
We cannot know this since the app does not have any information on the inputs.
There was a problem hiding this comment.
So which informations are we going to request the user to confirm, if the tx is a simple authority destroy? You usually confirm only the outputs?
How is the flow going to be?
There was a problem hiding this comment.
Sign tx confirmations are:
- User has to accept each output.
- Since there are no outputs, this would not show
- User has to accept sending the transaction.
So basically he would have to agree to send a transaction without outputs.
There was a problem hiding this comment.
Does it make sense to have a screen with "This transaction has no outputs" for the user to confirm?
|
|
||
| | Description | P1 | P2 | CData | | ||
| | ------------------------------------------- | ---- | ---- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Send token data<br>for signing transactions | 0x00 | 0x00 | `version (1)` \|\|<br> `uid (32)` \|\|<br>`symbol_len (1)` \|\|<br>`symbol (symbol_len)` \|\|<br>`name_len (1)` \|\|<br>`name (name_len)` \|\|<br>`signature (32)` | |
There was a problem hiding this comment.
What do you mean? Do you want to send the full transaction in bytes to the ledger, and we create a method to parse it there?
| [guide-level-explanation]: #guide-level-explanation | ||
|
|
||
| > [!WARNING] Request for comment | ||
| > This whole paragraph below can change if we really want to, we just need to add an `isNFT` boolean in the token data for the user to confirm, so in all transactions the Ledger will be informed that the token is an NFT with the user express consent during token signing |
There was a problem hiding this comment.
I would use a number of decimals parameter (instead of isNFT). In this case, NFTs would use decimals=0.
There was a problem hiding this comment.
Adding this new parameter will be addressed in another design since it requires some discussion on UI/UX (e.g. if we should add scrolling when using a lot of decimals).
rendered