-
Notifications
You must be signed in to change notification settings - Fork 302
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
[Coin Modularization] Cardano #6812
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
00de9c9
to
5b48fc3
Compare
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
5b48fc3
to
db91ef9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but a few questions because I might have missed some of the explanations for some logic in LLC 👍
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); | ||
}, | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🫤
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here I'm not quite sure to get why this should continue to live in LLC ?
0b7b664
to
b341e53
Compare
b341e53
to
e1a0339
Compare
e1a0339
to
d4c4a89
Compare
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
d4c4a89
to
f0d6ab8
Compare
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
* Sign Transaction with Ledger hardware | ||
*/ | ||
const buildSignOperation = | ||
(signerContext: SignerContext<CardanoSigner>): SignOperationFnSignature<Transaction> => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note but we could also type SignOperationFnSignature<CardanoAccount, Transaction>
* chore: define cardano in its own module Signed-off-by: Stéphane Prohaszka <[email protected]> * fix: lint Signed-off-by: Stéphane Prohaszka <[email protected]> * chore: update unimported Signed-off-by: Stéphane Prohaszka <[email protected]> * chore: feedbacks Signed-off-by: Stéphane Prohaszka <[email protected]> * fix: remove unnecessary test in favor of relying on type Signed-off-by: Stéphane Prohaszka <[email protected]> * fix: lint issue Signed-off-by: Stéphane Prohaszka <[email protected]> * feat: limit code in LLC by adding simpler serialiazer for signer Signed-off-by: Stéphane Prohaszka <[email protected]> * chore: simplify signOperation readability Signed-off-by: Stéphane Prohaszka <[email protected]> * chore: reorganize imports Signed-off-by: Stéphane Prohaszka <[email protected]> * fix: unimported and lock file Signed-off-by: Stéphane Prohaszka <[email protected]> --------- Signed-off-by: Stéphane Prohaszka <[email protected]>
✅ Checklist
npx changeset
was attached.📝 Description
Move Cardano...
❓ Context
🧐 Checklist for the PR Reviewers