Donjon code review

[adrienlacombe]

1. Instead of incrementing set the skip values right away so its more clear https://github.com/cometh-game/ledger-app-plugin/blob/f5047049ef85cd074101ae632d51d3e3917cee16/src/handle_init_contract.c#L56-L67
2. Sometimes memcpy is used other times copy_parameter. If there is no specific reason for this just use copy_parameter https://github.com/cometh-game/ledger-app-plugin/blob/f5047049ef85cd074101ae632d51d3e3917cee16/src/handle_provide_parameter.c#L68 https://github.com/cometh-game/ledger-app-plugin/blob/f5047049ef85cd074101ae632d51d3e3917cee16/src/handle_provide_parameter.c#L85
3. The rental_fee_token is never actually used when displaying information for the user. Remove all references to it since it is not needed.
4. In https://github.com/cometh-game/ledger-app-plugin/blob/f5047049ef85cd074101ae632d51d3e3917cee16/src/handle_provide_parameter.c#L211 we are copying a parameter to context->address but after two parameters are sent this value is overwritten https://github.com/cometh-game/ledger-app-plugin/blob/f5047049ef85cd074101ae632d51d3e3917cee16/src/handle_provide_parameter.c#L219. It is possible to just set the next parameter to RENTAL_NFT_TOKEN_ID or use the context->skip
5. handle_rental_end_rental and handle_rental_end_sublet are essentially the same function. Remove one of them
6. context->offset is never actually used. Removed all references to it, even in the context_t. The same applies for context->go_to_offset.
7. handle_provide_token.c file is not necessary you can remove it. And all references to the context->ticker/token_found/decimals should also be deleted.
8. Is it possible that in https://github.com/cometh-game/ledger-app-plugin/blob/f5047049ef85cd074101ae632d51d3e3917cee16/src/handle_query_contract_ui.c#L43 amountToString should be used instead of strlcpy.
9. Regarding this comment https://github.com/cometh-game/ledger-app-plugin/blob/f5047049ef85cd074101ae632d51d3e3917cee16/src/handle_query_contract_ui.c#L48 remove the comment and fix the title name if necessary.
10. https://github.com/cometh-game/ledger-app-plugin/blob/f5047049ef85cd074101ae632d51d3e3917cee16/src/main.c#L28-L58 move this into a separate file called contract.c

[jeje]

Fixes for the issues raised:

1. https://github.com/cometh-game/ledger-app-plugin/commit/f8850d263229d19d5a2b02db52b2c4ae8a65c176
2. https://github.com/cometh-game/ledger-app-plugin/commit/023d2b85fef44cb84bc7e3fcb11d5a6ba26deeff
3. See comment below
4. https://github.com/cometh-game/ledger-app-plugin/commit/bb11127d8455b758345e28373bb35a18fbb7340c
5. https://github.com/cometh-game/ledger-app-plugin/commit/cfcb5ad7dbb0dfae05c9fc35b6d58adaa3bda4fe
6. got rid of checkpoint as well: https://github.com/cometh-game/ledger-app-plugin/commit/a562721ae7ae7dfdcb15c3bc30053c03402017d9
7. See comment below
8. using amountToString(context->uint256_one, INT256_LENGTH, 0, "", msg->msg, msg->msgLength) makes the app to crash
9. this wasn't todo work but wrong wording; updated the comment in https://github.com/cometh-game/ledger-app-plugin/commit/c1756dbfa55d717b78dcb0e60c0e57f55e18a501
10. https://github.com/cometh-game/ledger-app-plugin/commit/74bea2fd4b4408bc14b6fcef0fbd34477e7a495d

Regarding 3. and 7. I'll try to modify the app so that is uses erc20OfInterest and handle_provider_token.c. Will post an update when this is working.

[jeje]

Fix for 3. and 7: https://github.com/cometh-game/ledger-app-plugin/commit/1320dc1c2be3f5fa6a0d53e002d7b107a0684633

As discussed with @adrienlacombe-ledger, the app will display Unknown token for the entry fee 'til MUST token is whitelisted.

[adrienlacombe]

hi @jeje few more comments from the Donjon

On https://github.com/cometh-game/ledger-app-plugin/blob/0086ac770b6d7ddbfb7574ccde77705bdf479abd/src/handle_provide_parameter.c#L122 and https://github.com/cometh-game/ledger-app-plugin/blob/0086ac770b6d7ddbfb7574ccde77705bdf479abd/src/handle_provide_parameter.c#L171 it is possible to trick the user by setting a tx with 0x10001 nfts, since it will be shown to the user that there is only 1 nft. A function could be created in order to read correctly this parameter, where it also makes sure that the rest of the parameter is zero.

https://github.com/cometh-game/ledger-app-plugin/blob/0086ac770b6d7ddbfb7574ccde77705bdf479abd/src/handle_query_contract_ui.c#L43 amountToString should be used. The app was crashing since an exception overflow was being thrown by the ethereum-plugin-sdk https://github.com/LedgerHQ/ethereum-plugin-sdk/blob/0de74c6382f876f89f82ef0eef90408fd94888a3/include/eth_internals.c#L203-L216 With the new commit https://github.com/cometh-game/ledger-app-plugin/commit/0086ac770b6d7ddbfb7574ccde77705bdf479abd the exception is caught and the signing process is aborted (meaning the app no longer crashes)

[jeje]

@adrienlacombe-ledger

• U2BE issue, fix here: https://github.com/cometh-game/ledger-app-plugin/commit/0bd732bf364b5793f47ee6aba513d1cb01392014
• amountToString is now not crashing the app, but the change still makes the test to fail for an unknown (to me) reason. As in that particular case we are dealing with integer values with no token symbol, I don't really understand why using amountToString is wise.

[jeje]

Merged welcome pack transactions and U4BE_from_parameter fix with this commit: https://github.com/cometh-game/ledger-app-plugin/commit/cd069240b284c60693666e3b84ea2b50d1ff1f68

[adrienlacombe]

hi @jeje minor update needed

• https://github.com/cometh-game/ledger-app-plugin/blob/cd069240b284c60693666e3b84ea2b50d1ff1f68/src/handle_provide_parameter.c#L247 context-address is never used, so consider removing this line

[jeje]

@adrienlacombe-ledger removed the superfluous code here: https://github.com/cometh-game/ledger-app-plugin/commit/789ae828634c4a30918c50a11961c7577f3e0bc0