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

[Kiln v2] Fix claim function signature #10

Merged
merged 19 commits into from
Sep 29, 2023

Conversation

loicttn
Copy link

@loicttn loicttn commented Sep 19, 2023

Claim function of the exit queue changed from claim(uint32[],uint32[],uint16) to claim(uint256[],uint32[],uint16) and multiClaim(address[],uint32[][],uint32[][]) to multiClaim(address[],uint256[][],uint32[][]), here is a PR to take this change into account (especially regarding the function selector).

@cedelavergne-ledger
Copy link

Hello,
1 check is fail, and expected workflows for Ledger guidelines_enforcer are not implemented

Can you please check on your side?

@loicttn
Copy link
Author

loicttn commented Sep 27, 2023

Hello, 1 check is fail, and expected workflows for Ledger guidelines_enforcer are not implemented

Can you please check on your side?

Hey, I am using the latest version of zemu in CI and tests dependencies, any idea why tests still timeout?

@agrojean-ledger
Copy link

Hello, 1 check is fail, and expected workflows for Ledger guidelines_enforcer are not implemented
Can you please check on your side?

Hey, I am using the latest version of zemu in CI and tests dependencies, any idea why tests still timeout?

@loicttn you need to change the name of plugin elf that should be loaded in test.fixture.js you have kiln instead of Kiln and the CI fails with Invalid lib name in /project/app/bin/plugin_nanox.elf (Kiln vs kiln)

@agrojean-ledger
Copy link

agrojean-ledger commented Sep 28, 2023

@loicttn I have tried with Kiln instead of kiln and the tests run but still fail...
You should configure the ethereum app it seems, before running your tests :

EthAppPleaseEnableContractData: Please enable Blind signing or Contract data in the Ethereum app Settings

You need something like that in your function zemu(device, func) { of test.fixture.js , before loading the plugin :

           if (dev.name == "stax") {
                const settingNav = new TouchNavigation([
                ButtonKind.InfoButton,
                ButtonKind.NavRightButton,
                ButtonKind.ToggleSettingButton1,
                ]);
                await sim.navigate(".", `${dev.prefix.toLowerCase()}-setting`, settingNav.schedule, true, false);
            } else {
                await sim.clickRight();
                await sim.clickBoth(undefined, false);
                await sim.clickBoth(undefined, false);
            }

@loicttn
Copy link
Author

loicttn commented Sep 28, 2023

@loicttn I have tried with Kiln instead of kiln and the tests run but still fail... You should configure the ethereum app it seems, before running your tests :

EthAppPleaseEnableContractData: Please enable Blind signing or Contract data in the Ethereum app Settings

You need something like that in your function zemu(device, func) { of test.fixture.js , before loading the plugin :

           if (dev.name == "stax") {
                const settingNav = new TouchNavigation([
                ButtonKind.InfoButton,
                ButtonKind.NavRightButton,
                ButtonKind.ToggleSettingButton1,
                ]);
                await sim.navigate(".", `${dev.prefix.toLowerCase()}-setting`, settingNav.schedule, true, false);
            } else {
                await sim.clickRight();
                await sim.clickBoth(undefined, false);
                await sim.clickBoth(undefined, false);
            }

So it seems that the claim function is not recognized now that we updated its signature 🤔 , if I understand correcly your solution is meant to enable blind signing which we want to avoid here, investigating why the claim selector is not recognized

@agrojean-ledger
Copy link

So it seems that the claim function is not recognized now that we updated its signature 🤔 , if I understand correcly your solution is meant to enable blind signing which we want to avoid here, investigating why the claim selector is not recognized

If you don't intend to use bling signing, don't use the solution I suggested.

@agrojean-ledger
Copy link

agrojean-ledger commented Sep 29, 2023

@loicttn you need to fix the app database :

Error:  Missing 'appFlags' for 'stax'
Checking manifest_nanosp.json
Checking for target 'nanos2' and variant 'kiln'
Retrieved listapploadparams:

As per the CI tip : Creating a PR on https://github.com/LedgerHQ/ledger-app-database to update the app-params-database.json

Then put me or someone else from our team and we'll merge the fix

loicttn added a commit to loicttn/ledger-app-database that referenced this pull request Sep 29, 2023
@cedelavergne-ledger
Copy link

Hello,

Currently, we accept errors in the workflow guidelines_enforcer, mainly because we still have pending work for the plugin case.
As soon as the E2E tests are ok, I will merge and start the deployment.
Just tell me when it is ok for the E2E tests (seems you only validate NANOS and NANOX. Is it sufficient?)

@loicttn
Copy link
Author

loicttn commented Sep 29, 2023

Hello,

Currently, we accept errors in the workflow guidelines_enforcer, mainly because we still have pending work for the plugin case. As soon as the E2E tests are ok, I will merge and start the deployment. Just tell me when it is ok for the E2E tests (seems you only validate NANOS and NANOX. Is it sufficient?)

E2E tests are all passing, so ok to merge thanks a lot 👍

@cedelavergne-ledger cedelavergne-ledger merged commit 2d8fb21 into LedgerHQ:develop Sep 29, 2023
24 of 28 checks passed
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.

5 participants