Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Donjon code review #19

Open
adrienlacombe opened this issue Feb 15, 2023 · 10 comments
Open

Donjon code review #19

adrienlacombe opened this issue Feb 15, 2023 · 10 comments

Comments

@adrienlacombe
Copy link

@adrienlacombe
Copy link
Author

@adrienlacombe
Copy link
Author

adrienlacombe commented Mar 16, 2023 •
edited
Loading

  • 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

    LedgerHQ-app-plugin-apwine/src/handle_provide_parameter.c

    Lines 76 to 109 in 8dffb87

    case PAIR_PATH_LENGTH: // _pairPath length
    handle_array_length(msg, context);
    context->next_param = PAIR_PATH_FIRST;
    break;
    case PAIR_PATH_FIRST: // _pairPath[0]
    handle_pair_path_first(msg, context);
    if (context->array_len <= 1) {
    context->skip = 1; // skip _tokenPath length
    context->next_param = TOKEN_PATH_SENT;
    } else {
    context->next_param = PAIR_PATH_LAST;
    }
    break;
    case PAIR_PATH_LAST: // _pairPath[length-1]
    handle_pair_path_last(msg, context);
    context->skip = 1; // skip _tokenPath length
    context->next_param = TOKEN_PATH_SENT;
    break;
    case TOKEN_PATH_SENT: // _tokenPath[0]
    handle_token_path_sent(msg, context);
    if (context->array_len <= 1) {
    context->next_param = TOKEN_PATH_RECEIVED;
    } else {
    context->skip = (context->array_len - 1) * 2 - 1; // Before the last tokenPath
    context->next_param = TOKEN_PATH_RECEIVED;
    }
    break;
    case TOKEN_PATH_RECEIVED: // _tokenPath[length-2]
    handle_token_path_received(msg, context);
    // When all parameters are parsed
    context->valid = 1;
    break;
    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

    LedgerHQ-app-plugin-apwine/src/apwine_plugin.h

    Lines 142 to 143 in 8dffb87

    uint16_t offset;
    uint16_t checkpoint;

@adrienlacombe
Copy link
Author

  • 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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

@adrienlacombe
Copy link
Author

@adrienlacombe
Copy link
Author 

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

  • According to the documentation in

    LedgerHQ-app-plugin-apwine/src/handle_query_contract_ui.c

    Lines 80 to 87 in ada361c

    // Set UI for retrieving the ticker.
    // The ticker is calculated based on the pair_path_first,pair_path_last and the token_path_received.
    // The pair 0 == (PT,Underlying) and 1 == (PT, FYT)
    // If the pair_path array length is greater than 1, it means that there are multiple swaps.
    // Therefore we will use the last pair_path to determine the ticker.
    // Otherwise, we will use the first pair_path.
    // If the token path is 0, the ticker is the PT ticker, and if the token path is 1,
    // the ticker is the Underlying or FYT ticker.
    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
Copy link
Author

  • 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
Copy link
Author

  • 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@adrienlacombe @GuilaneDen