Donjon code review

[adrienlacombe]

• Modify main according with https://github.com/LedgerHQ/app-plugin-nft/commit/12d0bc1dd210c068fe6d059ff90ed2cb8665f1f1

• Add the following to apwine_plugin.h https://github.com/LedgerHQ/app-plugin-boilerplate/blob/9faab4bed0fb0a57e4b60dc2acfd5a32ee4c5421/src/boilerplate_plugin.h#L61

• Use copy_address instead in https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/6c6bdd7c3ddcec2244061e561e519ad5f7a59248/src/handle_provide_parameter.c#L18-L29

• When when getting an offset/length from a parameter, one of these functions should be used https://github.com/LedgerHQ/ethereum-plugin-sdk/blob/81eb658b138f8201c666351315e79d04400219aa/include/eth_internals.h#L109-L111 , for instance in https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/6c6bdd7c3ddcec2244061e561e519ad5f7a59248/src/handle_provide_parameter.c#L34

• Looking at https://github.com/APWine/apwine-smart-contracts-public/blob/ec7468cd879bb245cb0ba2881e9df9141b8e80a3/amm/contracts/AMMRouterV1.sol#L69 , https://github.com/APWine/apwine-smart-contracts-public/blob/ec7468cd879bb245cb0ba2881e9df9141b8e80a3/amm/contracts/AMMRouterV1.sol#L149 it seems that length(_pairPath) <= 2, yet in https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/6c6bdd7c3ddcec2244061e561e519ad5f7a59248/src/handle_provide_parameter.c#L89-L91 allows it to have a value larger than 2, was this intended?

• When all parameters are parsed correctly, set context->valid = true and then on handle_finalize.c if context->valid == false set an error. It should be initialized to false before starting parsing parameters.

• Remove the following lines https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/6c6bdd7c3ddcec2244061e561e519ad5f7a59248/src/handle_provide_parameter.c#L179-L180 , this will make sure that if any other parameters are sent it will result in an error.

• The code should have been ready to be reviewed https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/6c6bdd7c3ddcec2244061e561e519ad5f7a59248/src/handle_finalize.c#L35-L40

[adrienlacombe]

• U2BE_from_parameter is now used but incorrectly, it returns a bool in case there is an error. set an error if that happens (there are other plugins that use this mechanism for instance app-plugin-paraswap)

• Looking at https://github.com/APWine/apwine-smart-contracts-public/blob/ec7468cd879bb245cb0ba2881e9df9141b8e80a3/amm/contracts/AMMRouterV1.sol#L69 , https://github.com/APWine/apwine-smart-contracts-public/blob/ec7468cd879bb245cb0ba2881e9df9141b8e80a3/amm/contracts/AMMRouterV1.sol#L149 it seems that length(_pairPath) <= 2, yet in https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/6c6bdd7c3ddcec2244061e561e519ad5f7a59248/src/handle_provide_parameter.c#L89-L91 allows it to have a value larger than 2, was this intended?

• When all parameters are parsed correctly, set context->valid = true and then on handle_finalize.c if context->valid == false set an error. It should be initialized to false before starting parsing parameters.

• Remove the following lines https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/6c6bdd7c3ddcec2244061e561e519ad5f7a59248/src/handle_provide_parameter.c#L179-L180 , this will make sure that if any other parameters are sent it will result in an error.

[adrienlacombe]

• The code that parses swapExactAmountIn, seems a bit confusing and is lacking basic checks and some essential parameters were not taken into account. Instead of having the code “jumping around between switch cases” like in https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/8dffb8714e005b2f3df41bf60d44400afa1be3d9/src/handle_provide_parameter.c#L76-L109 why not something like (the proposed code should be properly read and tested):

  case PAIR_PATH_LENGTH: if (!U2BE_from_parameter(msg->parameter, &(context->array_len)) && context->array_len == 0) { msg->result = ETH_PLUGIN_RESULT_ERROR; break; } context->tmp_len = context->array_len; context->next_param = PAIR_PATH_FIRST; break;

  case PAIR_PATH_FIRST: // _pairPath[0]
      context->tmp_len--;
  
      if (!U2BE_from_parameter(msg->parameter, &context->pair_path_first)) {
          msg->result = ETH_PLUGIN_RESULT_ERROR;
          break;
      }
  
      if (context->tmp_len == 0) {
          context->next_param = TOKEN_PATH_LENGTH;
      } else {
          context->skip = context->tmp_len - 1;
          context->next_param = PAIR_PATH_LAST;
      }
      break;

  case PAIR_PATH_LAST: // _pairPath[-1] if (!U2BE_from_parameter(msg->parameter, &context->pair_path_last)) { msg->result = ETH_PLUGIN_RESULT_ERROR; break; } context->next_param = TOKEN_PATH_LENGTH; break;

  case TOKEN_PATH_LENGTH:
      if (!U2BE_from_parameter(msg->parameter, &(context->tmp_len)) && context->tmp_len  * 2 != context->array_len) {
          msg->result = ETH_PLUGIN_RESULT_ERROR;
          break;
      }
      context->next_param = TOKEN_PATH_SENT;
      break;
  
  case TOKEN_PATH_SENT: // _tokenPath[0]
      if (!U2BE_from_parameter(msg->parameter, &(context->token_path_sent))) {
          msg->result = ETH_PLUGIN_RESULT_ERROR;
          break;
      }
      context->skip = context->tmp_len - 2;
      context->next_param = TOKEN_PATH_RECEIVED;
      break;
  
  case TOKEN_PATH_RECEIVED: // _tokenPath[-1]
      if (!U2BE_from_parameter(msg->parameter, &(context->token_path_received))) {
          msg->result = ETH_PLUGIN_RESULT_ERROR;
          break;
      }
      context->valid = 1;
      break;

• Also what is the need for https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/8dffb8714e005b2f3df41bf60d44400afa1be3d9/src/apwine_plugin.h#L142-L143

[adrienlacombe]

• Avoid code duplication, handle_add_liquidity and handle_remove_liquidity are the same. Use only one function with a new meaningful name.

• The function handle_zapintopt should make sure that the length of the _inputs array is != 0. Also what is the need for context->skip++;

• Avoid code duplication, the same functions are defined in different places, sent_network_token/received_network_token

• The code handle_swap_exact_tokens and handle_future_vault_tokens should set before the loop starts the decimals_sent/decimals_received to WEI_TO_ETHER.

[GuilaneDen]

Hi @adrienlacombe-ledger , thank you for the review.

I have a question about the handle_zapintopt function, the last parameter which is _input[1] is not a parameter that we are displaying. Therefore we don't want to parse it and skip it.

However, when I don't want to parse a certain parameter I put context->next_param = NONE or skip it with context->skip++; but it seems that neither of the two options is accepted by the donjon. Do you know how to skip a parameter at the end of the input data?

[adrienlacombe]

hi @GuilaneDen

When there is no need to parse more parameters we can use context->next_param = NONE or when we don’t want to parse certain parameters we can use context->skip = Y (to skip the next Y parameters). In the case of handle_zapintopt the length parameter of the array _inputs should be read and make sure that is a valid number. The correct way do code this function is to read the length of this array and make sure that is larger than 0 (at least). Then parse _inputs[0] and skip the remaining _inputs[1:] (python syntax to be clearer). Also set context->next_param = NONE since no more parameters are expected after skipping the remaining _inputs.

[adrienlacombe]

• In https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/b01fca3c8095719a5ec670a65e0f34c8d3851234/src/handle_provide_parameter.c#L83 should be context->tmp_len != context->array_len * 2

• Yarn test results in (HTTP code 404) no such container - No such image: zondax/builder-zemu@...

• Bad attribute in https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/b01fca3c8095719a5ec670a65e0f34c8d3851234/src/handle_query_contract_ui.c#L513

• It would be useful to have an explanation of set_send_ticker_ui/set_receive_ticker_ui or where to find documentation about it.

• set_send_ticker_ui/set_receive_ticker_ui/set_future_vault_token_deposit_ui/… should takeninto account if msg->msg is not updated with some ticker.

• Regarding the withdraw method, in https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/b01fca3c8095719a5ec670a65e0f34c8d3851234/src/handle_query_contract_ui.c#L282 , should it be “token receive”?

• Why this naming https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/b01fca3c8095719a5ec670a65e0f34c8d3851234/src/handle_query_contract_ui.c#L314-L319 ? Is there not a better naming?

• What is the need for beneficiary_screen?

[adrienlacombe]

• In the increase_amount when amount = 0; the token will be known and no warning screen will be displayed. Same for create_lock/add_liquidity/remove_liquidity.

• Beyond context->valid = 1, the plugin should raise an error in case there are unexpected parameters to be parsed. For example in https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/18c024bac17cb4d8eabfb9b41468fcadc196c763/src/handle_provide_parameter.c#L103 , https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/18c024bac17cb4d8eabfb9b41468fcadc196c763/src/handle_provide_parameter.c#L125 , https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/18c024bac17cb4d8eabfb9b41468fcadc196c763/src/handle_provide_parameter.c#L144 , https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/18c024bac17cb4d8eabfb9b41468fcadc196c763/src/handle_provide_parameter.c#L196 , https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/18c024bac17cb4d8eabfb9b41468fcadc196c763/src/handle_provide_parameter.c#L216 , https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/18c024bac17cb4d8eabfb9b41468fcadc196c763/src/handle_provide_parameter.c#L231 , context->next_param should be set to an error.

• In https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/18c024bac17cb4d8eabfb9b41468fcadc196c763/src/handle_provide_parameter.c#L172-L180 the code could be improved by checking the array_len on the INPUT_LENGHT case. Also the context->next_param should be set to an error and the skip should be set to array_len - 1.

• On handle_query_contract_ui.c in all functions that set a ticker, it is possible for the ticker to never actually be set. So the ticker could be set to a default value like “???” so that is displayed something is wrong to the user.

• On set_receive_ticker_ui the documentation should be improved regarding existing or not pair_path_last

[adrienlacombe]

• Not all the cases mentioned previously are fixed https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/18c024bac17cb4d8eabfb9b41468fcadc196c763/src/handle_provide_parameter.c#L103 , https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/18c024bac17cb4d8eabfb9b41468fcadc196c763/src/handle_provide_parameter.c#L125 , https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/18c024bac17cb4d8eabfb9b41468fcadc196c763/src/handle_provide_parameter.c#L144 , https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/18c024bac17cb4d8eabfb9b41468fcadc196c763/src/handle_provide_parameter.c#L196 , https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/18c024bac17cb4d8eabfb9b41468fcadc196c763/src/handle_provide_parameter.c#L216 , https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/18c024bac17cb4d8eabfb9b41468fcadc196c763/src/handle_provide_parameter.c#L231

• In the fix for handle_zapintopt what is the necessity of checking context->array_len != 0 in

if (context->array_len != 0) {
  handle_amount_received(msg, context);
} else {
  msg->result = ETH_PLUGIN_RESULT_ERROR;
  break;
}

• According to the documentation in https://github.com/blooo-io/LedgerHQ-app-plugin-apwine/blob/ada361c3614994fc369345a778b6577cfad2cb6c/src/handle_query_contract_ui.c#L80-L87 the following does not seem to be correct

• Instead of checking NULL_AMOUNT, why not set a flag marking that contract_address_sent/contract_address_received should be displayed to the user as unknown, for instance using a new flag.

• There seems to be repetitive functionality in handle_finalize and in handle_token. The function handle_finalize checks if the token is a network token or not. Again the same happens in handle_token. The token lookup mechanism should be improved/simplified. In handle_finalize it should only set tokenLookup/sent_network_token if the token is not marked as unknown(new flag). Then on handle_token it's only necessary to check if msg->item != null and if token_found is false set an additional screen. The default ticker could also be"?"

[adrienlacombe]

• There seems to be some confusion with the screens, for instance on INCREASE_AMOUNT the plugin sets both amount_sent and contract_sent_unknown and requests 0 screens. Since contract_sent_unknown is set, an additional screen is requested to display “Unknown token”. Yet the amount is still displayed and this seems related with the handle_token requesting an additional screen for the second token. This “pattern” seems to occur with the following cases: increase_amount, create_lock, redeem_yield (which requests 0 screens but due to handle_token it seems to request additional screens, then on get_screen the AMOUNT_SCREEN is returned …)

• INCERASE_UNLOCK_TIME <- fix typo

[adrienlacombe]

• Both ADD_LIQUIDITY/REMOVE_LIQUIDITY set contract_sent_unknown,contract_received_unknown=1, so what is the necessity of doing all the extra stuff in handle_liquidity_tokens. There should be an error in case of these not being set.

• Fix the compilation warnings

• Some tests are not passing