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

Finish hf v15 integration #1

Closed
wants to merge 10 commits into from

Conversation

fbeutin-ledger
Copy link

  • Add tests for view_tag instruction
  • Fix signature test with no view-tag
  • Propose fix for view_tag derivation

Copy link
Owner

@j-berman j-berman left a comment

Choose a reason for hiding this comment

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

the real fix should be in the tests, sending encrypted derivation instead?

Yep, the tests expect derivation to be encrypted with a dummy key 0x55.

Tests are passing on my end with the 2 suggestions.

@fbeutin-ledger fbeutin-ledger force-pushed the j-berman-hf-v15 branch 2 times, most recently from a4a03b0 to 4f59019 Compare July 25, 2022 09:19
THROW(SW_CLIENT_NOT_SUPPORTED);
// Check if version is supported
uint32_t i;
for (i = 0; i < MONERO_SUPPORTED_CLIENT_SIZE + 1; ++i) {
Copy link
Owner

Choose a reason for hiding this comment

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

In reference to: i < MONERO_SUPPORTED_CLIENT_SIZE + 1

When i == MONERO_SUPPORTED_CLIENT_SIZE, won't behavior be undefined when referencing supported_clients[i]? Seems like it's allowing 1 extra loop

Copy link
Owner

Choose a reason for hiding this comment

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

I think you meant this:

    uint32_t i;
    for (i = 0; i < MONERO_SUPPORTED_CLIENT_SIZE; ++i) {
        // Use strncmp to allow supported version prefixing client version
        unsigned int supported_clients_len = strlen((char*)PIC(supported_clients[i]));
        if (strncmp(PIC(supported_clients[i]), client_version, supported_clients_len) == 0) {
            break;
        }
    }
    if (i == MONERO_SUPPORTED_CLIENT_SIZE) {
        THROW(SW_CLIENT_NOT_SUPPORTED);
    }

Copy link
Author

Choose a reason for hiding this comment

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

Mmmh thanks for catching it, that is indeed a mistake.
I've separated the version check in a dedicated function, this should be better and more clear

@fbeutin-ledger fbeutin-ledger force-pushed the j-berman-hf-v15 branch 2 times, most recently from 0a6924e to 766425e Compare July 26, 2022 07:12
Copy link
Owner

@j-berman j-berman left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the changes!

@fbeutin-ledger
Copy link
Author

Thanks for the reviews, I've merged the changes on our develop branch

@lpascal-ledger lpascal-ledger deleted the j-berman-hf-v15 branch August 16, 2022 08:49
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.

2 participants