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.

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

Adding the Swap feature #6

Merged
merged 10 commits into from
Sep 27, 2023
Merged

Adding the Swap feature #6

merged 10 commits into from
Sep 27, 2023

Conversation

spalmer25
Copy link
Collaborator

@spalmer25 spalmer25 commented Sep 13, 2023

This PR enables the Ledger swap function:

  • Add ENABLE_SWAP tag
  • Provide the 4 required functions:
    • The swap_handle_check_address function which checks an address.
    • The swap_handle_get_printable_amount function, which gives the print format for an amount.
    • The swap_copy_transaction_parameters function, which backs up transaction parameters.
    • The swap_finalize_exchange_sign_transaction function which finalizes the exchange.
  • During a swap, check that the transaction matches the backed-up parameters.

I made some choices in the implementation:

  • In check_address: the derivation_type is set to ED25519 (since no parameter defines it).
  • In check_address/copy_transaction_parameters: the extra_id must be non-null and empty.
  • In the get_printable_amount function: the ticker and the number of decimals given by the caller are those used for print. Should we instead force the display with tz and 6 decimals?
  • In copy_transaction_parameters/check_validity: the ticker and the number of decimals are not taken into account, the amount/fee values are just compared without decimals.

The feature tests are on an LedgerHQ/app-exchange fork: spalmer25/app-exchange#1

@spalmer25 spalmer25 force-pushed the palmer/functori/swap-feature branch 6 times, most recently from 9c53f1e to 701485e Compare September 14, 2023 12:31
@spalmer25
Copy link
Collaborator Author

@spalmer25 spalmer25 marked this pull request as ready for review September 14, 2023 12:36
@spalmer25 spalmer25 force-pushed the palmer/functori/swap-feature branch 2 times, most recently from 7dcbd21 to 701485e Compare September 15, 2023 14:07
Copy link
Collaborator

@emturner emturner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall LGTM, will test manually against ledger live

app/src/handle_swap.c Outdated Show resolved Hide resolved
return true;
}

bool tz_print_uint64(uint64_t value, char *out, int out_len, int nb_decimals) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly something like tz_print_mutez instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is in lib_standard_app.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not use "print" in these function names, because they do not print.
tz_print_mutez() can be replaced by format_fpu64() from lib_standard_app in ledger-secure-sdk, I think.
If you are worried about building it outside for, say, tests, then use the approach in app/src/parser/compat.h and define:

bool
tz_format_mutez(uint64_t value, char *out, size_t outlen)
{
#ifdef ACTUALLY_ON_LEDGER
    return format_fpu64(out, outlen, value, 6);
#else
    size_t len = snprintf(out, outlen, "%u.%06u", value / 1000000, value % 1000000);
    return len < outlen;
#endif
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to add the above comment to my review, if I can.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!

@spalmer25 spalmer25 force-pushed the palmer/functori/swap-feature branch 3 times, most recently from c87447c to c8a268e Compare September 25, 2023 14:06
@spalmer25 spalmer25 force-pushed the palmer/functori/swap-feature branch 2 times, most recently from 300ecde to 1031475 Compare September 26, 2023 09:38
app/src/keys.c Outdated
@@ -152,6 +152,30 @@ cx_err_t public_key_hash(uint8_t *hash_out, size_t hash_out_size,
return error;
}

cx_err_t print_pkh(bip32_path_t bip32_path,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

print_pkh() is a misleading name because it does not print anything. I think that derive_pkh() is more meaningful because it actually does derive the public key before it hashes and formats it.

return true;
}

bool tz_print_uint64(uint64_t value, char *out, int out_len, int nb_decimals) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is in lib_standard_app.

app/src/keys.c Outdated
case DERIVATION_TYPE_SECP256R1: hash[0] = 2; break;
case DERIVATION_TYPE_ED25519:
case DERIVATION_TYPE_BIP32_ED25519: hash[0] = 0; break;
default: CX_CHECK(EXC_WRONG_PARAM); break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CX_CHECK should be TZ_FAIL here.

app/src/keys.h Outdated
@@ -67,3 +67,5 @@ check_derivation_type(derivation_type_t code)
{
return (code >= DERIVATION_TYPE_ED25519 && code < DERIVATION_TYPE_MAX);
}

void generate_pkh(derivation_type_t, const bip32_path_t *, char *, size_t);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would look better to put this prototype with the other prototypes after generate_public_key().

app/src/keys.c Outdated
@@ -134,6 +134,33 @@ public_key_hash(uint8_t *hash_out, size_t hash_out_size,
TZ_POSTAMBLE;
}

void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function above (public_key_hash()) should be static as we've removed its prototype and it therefore should not be accessible outside this C file.

Copy link
Contributor

@elric1 elric1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed another quick one. This should simplify things quite a lot.

Comment on lines 135 to 154
bool
tz_print_mutez(uint64_t value, char *out, int out_len, int nb_decimals)
{
char buff[TZ_NUM_BUFFER_SIZE] = {0};
uint64_t power = 10;
int offset = 0;

while (power <= value)
power *= 10;

while (power != 1) {
power /= 10;
buff[offset++] = value / power + '0';
value %= power;
}

buff[offset] = '\0';

return tz_adjust_decimal(buff, offset, out, out_len, nb_decimals);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates functionality that is already defined in ledger-secure-sdk/lib_standard_app/format.c. This function should be able to be rewritten something like this:

bool
tz_print_mutez2(uint64_t value, char *out, int out_len, int nb_decimals)
{
#if ACTUALLY_ON_LEDGER
    format_fpu64(out, out_len, value, nb_decimals);
#else
    size_t divisor = 1;
    size_t i;
    size_t ret;

    for (i=0; i < nb_decimals; i++)
        divisor *= 10;

    ret = snprintf(out, out_len, "%u.%0*u",
                   value / divisor,  nb_decimals, value % divisor);
    return ret < out_len;
#endif
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACTUALLY_ON_LEDGER can be found in app/src/parser/compat.h which is how you decide if you are inside the unit tests.
My function assumes that if you are not on a ledger, then you are on an LP64 box with a functioning snprintf(3) in libc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is only useful in swap, so it would be better not to have this function in parser at all and to use the functions from ledger-secure-sdk/lib_standard_app/format.c directly.

@spalmer25 spalmer25 force-pushed the palmer/functori/swap-feature branch from 1031475 to 57de80a Compare September 27, 2023 08:12
@spalmer25 spalmer25 requested a review from elric1 September 27, 2023 08:15
@spalmer25 spalmer25 force-pushed the palmer/functori/swap-feature branch from 57de80a to 0df2f64 Compare September 27, 2023 08:48
@spalmer25 spalmer25 force-pushed the palmer/functori/swap-feature branch from 0df2f64 to 4496155 Compare September 27, 2023 08:53
@elric1 elric1 merged commit 3783dd0 into main Sep 27, 2023
87 checks passed
@elric1 elric1 deleted the palmer/functori/swap-feature branch September 27, 2023 09:28
ajinkyaraj-23 pushed a commit that referenced this pull request Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants