[Coin Modularization] Cardano #6812

sprohaszka-ledger · 2024-05-06T17:36:32Z

✅ Checklist

npx changeset was attached.
Covered by automatic tests.
Impact of the changes:
- ...

📝 Description

Move Cardano...

❓ Context

JIRA or GitHub link: LIVE-8166

🧐 Checklist for the PR Reviewers

The code aligns with the requirements described in the linked JIRA or GitHub issue.
The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
There are no undocumented trade-offs, technical debt, or maintainability issues.
The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
Any new dependencies have been justified and documented.
Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

vercel · 2024-05-06T17:36:41Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
web-tools	❌ Failed (Inspect)			May 23, 2024 8:26pm

4 Ignored Deployments

Name	Status	Preview	Updated (UTC)
ledger-live-docs	⬜️ Ignored (Inspect)	Visit Preview	May 23, 2024 8:26pm
ledger-live-github-bot	⬜️ Ignored (Inspect)	Visit Preview	May 23, 2024 8:26pm
native-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	May 23, 2024 8:26pm
react-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	May 23, 2024 8:26pm

socket-security · 2024-05-07T13:09:27Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@ledgerhq/coin-cardano@0.0.1	None	`0`	0 B

View full report↗︎

libs/coin-modules/coin-cardano/src/bridge/js.ts

libs/coin-modules/coin-cardano/src/cli-transaction.ts

libs/coin-modules/coin-cardano/src/js-signOperation.ts

libs/coin-modules/coin-cardano/src/logic.ts

libs/coin-modules/coin-cardano/src/types.ts

libs/ledger-live-common/src/generated/types.ts

lambertkevin

LGTM, but a few questions because I might have missed some of the explanations for some logic in LLC 👍

lambertkevin · 2024-05-17T12:49:44Z

libs/ledger-live-common/src/families/cardano/setup.ts

+  return {
+    getAddress: async ({
+      path,
+      stakingPathString,
+      networkParams,
+      verify,
+    }: GetAddressRequest): Promise<CardanoAddress> => {
+      const network =
+        networkParams.networkId === Networks.Mainnet.networkId
+          ? Networks.Mainnet
+          : Networks.Testnet;
+
+      const r = await ada.deriveAddress({
+        network,
+        address: {
+          type: AddressType.BASE_PAYMENT_KEY_STAKE_KEY,
+          params: {
+            spendingPath: str_to_path(path),
+            stakingPath: str_to_path(stakingPathString),
+          },
+        },
+      });
+      if (verify) {
+        await ada.showAddress({
+          network,
+          address: {
+            type: AddressType.BASE_PAYMENT_KEY_STAKE_KEY,
+            params: {
+              spendingPath: str_to_path(path),
+              stakingPath: str_to_path(stakingPathString),
+            },
+          },
+        });
+      }
+      const address = TyphonUtils.getAddressFromHex(r.addressHex) as TyphonAddress.BaseAddress;
+      return {
+        address: address.getBech32(),
+        // Here, we use publicKey hash, as cardano app doesn't export the public key
+        publicKey: address.paymentCredential.hash,
+      };
+    },
+    getPublicKey: async (accountPath: string): Promise<CardanoExtendedPublicKey> => {
+      return ada.getExtendedPublicKey({
+        path: str_to_path(accountPath),
+      });
+    },
+    sign: async ({
+      unsignedTransaction,
+      accountPubKey,
+      accountIndex,
+      networkParams,
+    }: CardanoSignRequest): Promise<CardanoSignature> => {
+      const rawInputs = unsignedTransaction.getInputs();
+      const ledgerAppInputs = rawInputs.map(i => prepareLedgerInput(i, accountIndex));
+
+      const rawOutptus = unsignedTransaction.getOutputs();
+      const ledgerAppOutputs = rawOutptus.map(o => prepareLedgerOutput(o, accountIndex));
+
+      const rawCertificates = unsignedTransaction.getCertificates();
+      const ledgerCertificates = rawCertificates.map(prepareCertificate);
+
+      const rawWithdrawals = unsignedTransaction.getWithdrawals();
+      const ledgerWithdrawals = rawWithdrawals.map(prepareWithdrawal);
+
+      const auxiliaryDataHashHex = unsignedTransaction.getAuxiliaryDataHashHex();
+
+      const network =
+        networkParams.networkId === Networks.Mainnet.networkId
+          ? Networks.Mainnet
+          : Networks.Testnet;
+
+      const trxOptions: SignTransactionRequest = {
+        signingMode: TransactionSigningMode.ORDINARY_TRANSACTION,
+        tx: {
+          network,
+          inputs: ledgerAppInputs,
+          outputs: ledgerAppOutputs,
+          certificates: ledgerCertificates,
+          withdrawals: ledgerWithdrawals,
+          fee: unsignedTransaction.getFee().toString(),
+          ttl: unsignedTransaction.getTTL()?.toString(),
+          validityIntervalStart: null,
+          auxiliaryData: auxiliaryDataHashHex
+            ? {
+                type: TxAuxiliaryDataType.ARBITRARY_HASH,
+                params: {
+                  hashHex: auxiliaryDataHashHex,
+                },
+              }
+            : null,
+        },
+        additionalWitnessPaths: [],
+      };
+
+      const r = await ada.signTransaction(trxOptions);
+      return signTx(unsignedTransaction, accountPubKey, r.witnesses);
+    },
+  };
+};


Feels kinda wrong to have that much coin logic into LLC don't you think ? 🤔
Really feels like the coin-module would be unusable without LLC here 🫤

I know, but what is bothering me is that all this logic is relying on @cardano-foundation/ledgerjs-hw-app-cardano, which relies itself on @ledgerhq/hw-transport (forbidden to use in CoinModule).

lambertkevin · 2024-05-17T12:53:42Z

libs/ledger-live-common/src/families/cardano/staking.ts

+export type { APIGetPoolsDetail, StakePool } from "@ledgerhq/coin-cardano/api/api-types";
+export { fetchPoolDetails } from "@ledgerhq/coin-cardano/api/getPools";
+export { LEDGER_POOL_IDS } from "@ledgerhq/coin-cardano/utils";


Is this necessary ? Can't UI specific to Cardano simply get the types from @ledgerhq/coin-cardano directly ? Or do you specifically want that to be an entrypoint for those types & constants ?

I prefer to separate UI from CoinModules direct use. This is why I keep in LLC some entrypoint to access the coin-modules.
Ideally, the logic required by the UI should be LLC. The UI shouldn't have any clue about what the meaning of what it displays.

lambertkevin · 2024-05-17T12:54:59Z

libs/ledger-live-common/src/families/cardano/tx-helpers.ts

Same thing here I'm not quite sure to get why this should continue to live in LLC ?

Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr>

lvndry · 2024-05-24T15:11:29Z

libs/coin-modules/coin-cardano/src/js-signOperation.ts

+ * Sign Transaction with Ledger hardware
+ */
+const buildSignOperation =
+  (signerContext: SignerContext<CardanoSigner>): SignOperationFnSignature<Transaction> =>


Just a note but we could also type SignOperationFnSignature<CardanoAccount, Transaction>

* chore: define cardano in its own module Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr> * fix: lint Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr> * chore: update unimported Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr> * chore: feedbacks Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr> * fix: remove unnecessary test in favor of relying on type Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr> * fix: lint issue Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr> * feat: limit code in LLC by adding simpler serialiazer for signer Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr> * chore: simplify signOperation readability Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr> * chore: reorganize imports Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr> * fix: unimported and lock file Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr> --------- Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr>

github-actions bot assigned sprohaszka-ledger May 6, 2024

live-github-bot bot added desktop Has changes in LLD mobile Has changes in LLM common Has changes in live-common ledgerjs Has changes in the ledgerjs open source libs labels May 6, 2024

vercel bot had a problem deploying to Preview – web-tools May 6, 2024 17:41 Failure

sprohaszka-ledger force-pushed the feat/LIVE-8166-cardano branch from 00de9c9 to 5b48fc3 Compare May 7, 2024 13:07

vercel bot deployed to Preview – web-tools May 7, 2024 13:14 View deployment

lvndry reviewed May 13, 2024

View reviewed changes

sprohaszka-ledger force-pushed the feat/LIVE-8166-cardano branch from 5b48fc3 to db91ef9 Compare May 14, 2024 11:48

sprohaszka-ledger marked this pull request as ready for review May 14, 2024 11:49

sprohaszka-ledger requested review from a team as code owners May 14, 2024 11:49

vercel bot deployed to Preview – web-tools May 14, 2024 11:55 View deployment

vercel bot deployed to Preview – web-tools May 14, 2024 12:29 View deployment

lvndry previously approved these changes May 17, 2024

View reviewed changes

sprohaszka-ledger dismissed lvndry’s stale review via 0b7b664 May 17, 2024 11:03

vercel bot deployed to Preview – react-ui-storybook May 17, 2024 11:05 View deployment

vercel bot deployed to Preview – web-tools May 17, 2024 11:10 View deployment

lambertkevin previously approved these changes May 17, 2024

View reviewed changes

sprohaszka-ledger dismissed lambertkevin’s stale review via b341e53 May 21, 2024 08:03

sprohaszka-ledger force-pushed the feat/LIVE-8166-cardano branch from 0b7b664 to b341e53 Compare May 21, 2024 08:03

vercel bot had a problem deploying to Preview – web-tools May 21, 2024 08:06 Failure

sprohaszka-ledger force-pushed the feat/LIVE-8166-cardano branch from b341e53 to e1a0339 Compare May 21, 2024 08:14

vercel bot deployed to Preview – web-tools May 21, 2024 08:22 View deployment

sprohaszka-ledger force-pushed the feat/LIVE-8166-cardano branch from e1a0339 to d4c4a89 Compare May 21, 2024 13:40

vercel bot had a problem deploying to Preview – web-tools May 21, 2024 13:47 Failure

chore: define cardano in its own module

06ecfc6

Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr>

sprohaszka-ledger added 6 commits May 23, 2024 13:46

fix: lint

12d8982

Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr>

chore: update unimported

faee5ba

Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr>

chore: feedbacks

d8d8bb5

Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr>

fix: remove unnecessary test in favor of relying on type

b2cc881

Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr>

fix: lint issue

5dfa1bd

Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr>

feat: limit code in LLC by adding simpler serialiazer for signer

f0d6ab8

Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr>

sprohaszka-ledger force-pushed the feat/LIVE-8166-cardano branch from d4c4a89 to f0d6ab8 Compare May 23, 2024 16:26

vercel bot had a problem deploying to Preview – web-tools May 23, 2024 16:33 Failure

chore: simplify signOperation readability

ca88d10

Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr>

vercel bot had a problem deploying to Preview – web-tools May 23, 2024 19:49 Failure

chore: reorganize imports

3cae01a

Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr>

vercel bot had a problem deploying to Preview – web-tools May 23, 2024 20:08 Failure

fix: unimported and lock file

5612985

Signed-off-by: Stéphane Prohaszka <stephane.prohaszka@ledger.fr>

vercel bot had a problem deploying to Preview – web-tools May 23, 2024 20:26 Failure

lvndry approved these changes May 24, 2024

View reviewed changes

sprohaszka-ledger merged commit cec1599 into develop May 24, 2024
57 of 58 checks passed

sprohaszka-ledger deleted the feat/LIVE-8166-cardano branch May 24, 2024 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Coin Modularization] Cardano #6812

[Coin Modularization] Cardano #6812

sprohaszka-ledger commented May 6, 2024 •

edited by jira bot

vercel bot commented May 6, 2024 •

edited

socket-security bot commented May 7, 2024 •

edited

lambertkevin left a comment

lambertkevin May 17, 2024

sprohaszka-ledger May 20, 2024

lambertkevin May 17, 2024

sprohaszka-ledger May 20, 2024

lambertkevin May 17, 2024

lvndry May 24, 2024

[Coin Modularization] Cardano #6812

[Coin Modularization] Cardano #6812

Conversation

sprohaszka-ledger commented May 6, 2024 • edited by jira bot

✅ Checklist

📝 Description

❓ Context

🧐 Checklist for the PR Reviewers

vercel bot commented May 6, 2024 • edited

socket-security bot commented May 7, 2024 • edited

lambertkevin left a comment

Choose a reason for hiding this comment

lambertkevin May 17, 2024

Choose a reason for hiding this comment

sprohaszka-ledger May 20, 2024

Choose a reason for hiding this comment

lambertkevin May 17, 2024

Choose a reason for hiding this comment

sprohaszka-ledger May 20, 2024

Choose a reason for hiding this comment

lambertkevin May 17, 2024

Choose a reason for hiding this comment

lvndry May 24, 2024

Choose a reason for hiding this comment

sprohaszka-ledger commented May 6, 2024 •

edited by jira bot

vercel bot commented May 6, 2024 •

edited

socket-security bot commented May 7, 2024 •

edited