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

feat: fix logic and refactor app install #8401

Merged
merged 11 commits into from
Nov 25, 2024
Merged

feat: fix logic and refactor app install #8401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
46a7eed
feat: fix logic and refactor
chrisduma-ledger Nov 19, 2024
5c4455f
feat: add changeset
chrisduma-ledger Nov 19, 2024
e383880
Merge branch 'develop' of github.com:LedgerHQ/ledger-live into feat/s…
chrisduma-ledger Nov 19, 2024
a6ac170
fix: rm clog
chrisduma-ledger Nov 19, 2024
fa11bae
fix: add avax-like apps to use eth app
chrisduma-ledger Nov 20, 2024
7083655
Merge branch 'develop' of github.com:LedgerHQ/ledger-live into feat/s…
chrisduma-ledger Nov 20, 2024
b9bf19b
:Merge branch 'develop' of github.com:LedgerHQ/ledger-live into feat/…
chrisduma-ledger Nov 20, 2024
4000bdb
Merge branch 'develop' of github.com:LedgerHQ/ledger-live into feat/s…
chrisduma-ledger Nov 21, 2024
1fbfcc9
feat: make sure accountId is there
chrisduma-ledger Nov 22, 2024
5d823c9
:wq
chrisduma-ledger Nov 22, 2024
b6d0d62
Merge branch 'develop' of github.com:LedgerHQ/ledger-live into feat/s…
chrisduma-ledger Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/young-coats-flash.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"ledger-live-desktop": minor
"live-mobile": minor
"@ledgerhq/live-common": minor
"@ledgerhq/wallet-api-exchange-module": minor
---

Fixes app install and refactors logic
3 changes: 2 additions & 1 deletion libs/exchange-module/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,13 @@ export class ExchangeModule extends CustomModule {
*
* @returns - A transaction ID used to complete the exchange process
*/
async startSell({ provider }: Omit<ExchangeStartSellParams, "exchangeType">) {
async startSell({ provider, fromAccountId }: Omit<ExchangeStartSellParams, "exchangeType">) {
const result = await this.request<ExchangeStartSellParams, ExchangeStartResult>(
"custom.exchange.start",
{
exchangeType: "SELL",
provider,
fromAccountId,
},
);

Expand Down
1 change: 1 addition & 0 deletions libs/exchange-module/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export type ExchangeStartFundParams = {
export type ExchangeStartSellParams = {
exchangeType: "SELL";
provider: string;
fromAccountId: string;
};

export type ExchangeStartSwapParams = {
Expand Down
56 changes: 26 additions & 30 deletions libs/ledger-live-common/src/hw/actions/startExchange.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,38 +129,34 @@ export const createAction = (
appName: "Exchange",
};
}
if (!exchange || !mainFromAccount || !mainToAccount) {
return {
appName: "Exchange",
requireLatestFirmware,
};
} else {
const shouldAddEthApp =
(mainFromAccount.currency.family === "evm" || mainToAccount.currency.family === "evm") &&
mainFromAccount.currency.managerAppName !== "Ethereum" &&
mainToAccount.currency.managerAppName !== "Ethereum";
Comment on lines -138 to -141
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you removed a recent fix I did here, any reason we removed it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello. Yes, I have removed that and just added this part:
dependencies.push({ appName: mainFromAccount?.currency?.managerAppName });

It makes the code easier to read, and adds any app as needed. @CremaFR tested it for swaps, and it works well.

Copy link
Contributor

@Justkant Justkant Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes but we need to keep the above logic to add ethereum in case of avax as it's needed for swap
And instead of doing a specific logic for only one coin we do a semi-generic thing here to add ethereum in the deps for evm families
https://ledgerhq.atlassian.net/browse/LIVE-14666

Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a comment next time to explain such changes? This was incredibly obscure, and no one could have guessed it without context.
Even just linking the JIRA ticket would have been enough, as we often do for similar quirks in swaps https://github.com/LedgerHQ/ledger-live/blob/develop/apps/ledger-live-desktop/src/renderer/screens/exchange/Swap2/Form/index.tsx#L388

Copy link
Member

Choose a reason for hiding this comment

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

Or put in a function like addAvaxAppDep

Copy link
Member

Choose a reason for hiding this comment

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

And I'll make a PR to add PTX owner of this file too from now on

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes sure, the name of the variable and the logic was not that hard to follow IMO and I understand that you probably wanted to remove this but if something is obscure or doesn't make sense I usually find the author of the change and try to get answers before doing a change if none are provided in the source code
I'll keep this in mind if I end up in a similar situation, as I usually add context if needed for strange changes

Copy link
Member

Choose a reason for hiding this comment

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

You're right, could’ve reached out first. Now that I know about managerAppName not being reliable for swap, it makes sense.
but seeing appManagerName to use directly instead of getting accounts like

const dependencies: AppRequest["dependencies"] = [
          {
            account: mainFromAccount,
          },
          {
            account: mainToAccount,
          },
        ];

I assume your fix was an issue with accounts not installing correct apps and you just didn't refacto the thing, and that we should just have used managerAppName directly...

anyway invited you to #ptx-pr-review , it's the kind of changes I like to follow and know about 😹

const dependencies: AppRequest["dependencies"] = [
{
account: mainFromAccount,
},
{
account: mainToAccount,
},
];

if (shouldAddEthApp) {
dependencies.push({
appName: "Ethereum",
});
}
const dependencies: AppRequest["dependencies"] = [];
if (mainFromAccount) {
dependencies.push({ appName: mainFromAccount?.currency?.managerAppName });
}

return {
appName: "Exchange",
dependencies,
requireLatestFirmware,
};
if (mainToAccount) {
dependencies.push({ appName: mainToAccount?.currency?.managerAppName });
}

const shouldAddEthApp =
(mainFromAccount?.currency?.family === "evm" ||
mainToAccount?.currency?.family === "evm") &&
mainFromAccount?.currency?.managerAppName !== "Ethereum" &&
mainToAccount?.currency?.managerAppName !== "Ethereum";

// Check if we should add ETH app, for cases like when we want AVAX to use the ETH app.
if (shouldAddEthApp) {
dependencies.push({
appName: "Ethereum",
});
}
}, [exchange, mainFromAccount, mainToAccount, requireLatestFirmware]);

return {
appName: "Exchange",
dependencies,
requireLatestFirmware,
};
}, [mainFromAccount, mainToAccount, requireLatestFirmware]);

const appState = createAppAction(connectAppExec).useHook(reduxDeviceFrozen, request);

Expand Down
1 change: 1 addition & 0 deletions libs/ledger-live-common/src/wallet-api/Exchange/server.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ describe("handlers", () => {
const params: ExchangeStartSellParams = {
exchangeType: "SELL",
provider: "TestSellProvider",
fromAccountId: accounts[0].id,
};
const { request, context, walletHandlers } = prepareSellRequest(params);

Expand Down
33 changes: 31 additions & 2 deletions libs/ledger-live-common/src/wallet-api/Exchange/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ type ExchangeStartParamsUiRequest =
| {
exchangeType: "SELL";
provider: string;
exchange: Partial<Exchange> | undefined;
}
| {
exchangeType: "SWAP";
Expand Down Expand Up @@ -130,7 +131,7 @@ export const handlers = ({
if (params.exchangeType == "SWAP") {
exchangeParams = extractSwapStartParam(params, accounts);
} else if (params.exchangeType == "SELL") {
exchangeParams = extractSellStartParam(params);
exchangeParams = extractSellStartParam(params, accounts);
} else {
exchangeParams = {
exchangeType: params.exchangeType,
Expand Down Expand Up @@ -362,13 +363,41 @@ function extractSwapStartParam(
};
}

function extractSellStartParam(params: ExchangeStartSellParams): ExchangeStartParamsUiRequest {
function extractSellStartParam(
params: ExchangeStartSellParams,
accounts: AccountLike[],
): ExchangeStartParamsUiRequest {
if (!("provider" in params)) {
throw new ExchangeError(createWrongSellParams(params));
}

if (!params.fromAccountId) {
return {
exchangeType: params.exchangeType,
provider: params.provider,
} as ExchangeStartParamsUiRequest;
}

const realFromAccountId = getAccountIdFromWalletAccountId(params?.fromAccountId);

if (!realFromAccountId) {
throw new ExchangeError(createAccounIdNotFound(params.fromAccountId));
}

const fromAccount = accounts?.find(acc => acc.id === realFromAccountId);

if (!fromAccount) {
throw new ServerError(createAccountNotFound(params.fromAccountId));
}

const fromParentAccount = getParentAccount(fromAccount, accounts);

return {
exchangeType: params.exchangeType,
provider: params.provider,
exchange: {
fromAccount,
fromParentAccount,
},
};
}
Loading