Ledger Donjon review

[adrienlacombe]

• Previous issue: The function handle_swap_dodo_route_proxy_dodo_mutli_swap could have more suggestive names in the switch statement. Also when getting an offset from a parameter it one of these functions should be used https://github.com/LedgerHQ/ethereum-plugin-sdk/blob/81eb658b138f8201c666351315e79d04400219aa/include/eth_internals.h#L109-L111 https://github.com/LedgerHQ/ethereum-plugin-sdk/blob/81eb658b138f8201c666351315e79d04400219aa/include/eth_internals.h#L109-L111 . Also does not seem necessary to be decrementing SELECTOR_SIZE to calculate the offset and them adding SELECTOR_SIZE when checking if the parameterOffset is the wanted offset. Other than that on PATH_LENGTH it should be checked the length of the array

• main.c should take into account these changes https://github.com/LedgerHQ/app-plugin-nft/commit/12d0bc1dd210c068fe6d059ff90ed2cb8665f1f1

• Add context->valid = true inhttps://github.com/DODOEX/ledger-plugin/blob/cb7433f891fd03d7452640a0f8d2298f70aff7cc/src/handle_provide_parameter.c#L28 , https://github.com/DODOEX/ledger-plugin/blob/cb7433f891fd03d7452640a0f8d2298f70aff7cc/src/handle_provide_parameter.c#L57 , https://github.com/DODOEX/ledger-plugin/blob/cb7433f891fd03d7452640a0f8d2298f70aff7cc/src/handle_provide_parameter.c#L82 , etc (basically after parsing the wanted fields) and then on plugin finalize if context->valid == false set an error

• In https://github.com/DODOEX/ledger-plugin/blob/cb7433f891fd03d7452640a0f8d2298f70aff7cc/src/handle_query_contract_ui.c#L22 it is possible to write more data than the buffer ticker can hold

• For clarity when swapping a token for eth in https://github.com/DODOEX/ledger-plugin/blob/cb7433f891fd03d7452640a0f8d2298f70aff7cc/src/handle_provide_parameter.c#L82 its possible to add memcpy(context->token_received, ETH_ADDRESS/NULL_ADDR, ADDRESS_LENGTH), same in handle_swap_v2_proxy_dodo_swap_v2_eth_to_token with `context->token_pay

• For the withdraw method there could be a single screen with the name withdraw and the amount

• For the deposit method there could be a single screen with the name deposit and the amount

[junjieit]

The function handle_swap_dodo_route_proxy_dodo_mutli_swap could have more suggestive names in the switch statement

We are named by method name.

Also when getting an offset from a parameter it one of these functions should be used...

Here is the use of U2BE.

Other than that on PATH_LENGTH it should be checked the length of the array

It means to judge whether the length of msg->parameter is PARAMETER_LENGTH?

Add context->valid = true...

is it necessary to add this?

For the withdraw method there could be a single screen with the name withdraw and the amount For the deposit method there could be a single screen with the name deposit and the amount

Because here is the swap page on the front-end page, swap eth => weth or weth => eth. so there is no single screen display

[adrienlacombe]

The function handle_swap_dodo_route_proxy_dodo_mutli_swap could have more suggestive names in the switch statement

We are named by method name.

This is not a real issue but don’t see the meaning of the names PATH_OFFSET/PATH_LENGTH since the arguments of the method dodoMutliSwap don’t have a argument called path. This would improved readability

Also when getting an offset from a parameter it one of these functions should be used...

Here is the use of U2BE.

For instance https://github.com/LedgerHQ/app-plugin-rarible/commit/60cb195f47c0f68d53917f52e53c1e503b0f4f99

Other than that on PATH_LENGTH it should be checked the length of the array

It means to judge whether the length of msg->parameter is PARAMETER_LENGTH?

For instance checking if it has a meaningful value like != 0

Add context->valid = true...

is it necessary to add this?

Yes, otherwise not sending any parameters would lead to use of “uninitialized” values and therefore wrong information displayed to the user.

For the withdraw method there could be a single screen with the name withdraw and the amount For the deposit method there could be a single screen with the name deposit and the amount

Because here is the swap page on the front-end page, swap eth => weth or weth => eth. so there is no single screen display

[adrienlacombe]

• This is not a real issue but don’t see the meaning of the names PATH_OFFSET/PATH_LENGTH since the arguments of the method dodoMutliSwap don’t have a argument called path. This would improved readability

• For instance https://github.com/LedgerHQ/app-plugin-rarible/commit/60cb195f47c0f68d53917f52e53c1e503b0f4f99 now uses the new U2BE/U4BE functions from parameters

• For instance checking if it has a meaningful value like != 0

• Yes, otherwise not sending any parameters would lead to use of “uninitialized” values and therefore wrong information displayed to the user.