Local and Ledger wallet support #167
Conversation
| // If execute flag is provided, a signer cannot, as the Keypair is required | ||
| const isInvalidSigner = !!this.flags.execute && !!this.flags.signer | ||
| this.require(!isInvalidSigner, 'To execute the transaction the signer must be loaded from a wallet') | ||
| const signer = new PublicKey(this.flags.signer || this.wallet.publicKey) |
There was a problem hiding this comment.
Not executing but preparing a rawTx for a specific signer - Do we even need this functionality anymore?
There was a problem hiding this comment.
I don't think we need it
| import { Message, Transaction } from '@solana/web3.js' | ||
| import { CONTRACT_LIST } from '@chainlink/gauntlet-solana-contracts' | ||
|
|
||
| export default class SendRawTx extends SolanaCommand { |
There was a problem hiding this comment.
Hmm, why is this part of gauntlet-serum-multisig?
IMO this base command should be exposed from guntlet-solana.
There was a problem hiding this comment.
Right. Was there for testing purposes only
| return await sendAndConfirmRawTransaction(this.provider.connection, signedTx.serialize()) | ||
| } | ||
|
|
||
| withIDL = (action: (...args: any) => Promise<TransactionSignature>, idl: Idl) => async ( |
There was a problem hiding this comment.
Hmm, calling this method doesn't read very well to me:
const txhash = await this.withIDL(this.signAndSendRawTx, program.idl)(rawTxs, [feed])
There was a problem hiding this comment.
We always send the same this.signAndSendRawTx action in, so makes sense for this fn to be restructured slightly based on its demonstrated usage.
There was a problem hiding this comment.
Hmm, I don't want to mess up signAndSendRawTx with contract related stuff, as is quite clear what it does. Changing the naming for sendTxWithIDL sounds better?
| try { | ||
| return await action(...args) | ||
| } catch (e) { | ||
| console.error(e) |
There was a problem hiding this comment.
Will this printout cause duplicated error logs, as this function logs and throws again to be handled.
Consider removing this line.
| @@ -1 +1,3 @@ | |||
| export const ADDRESS_ZERO = 'solana_address_zero' | |||
|
|
|||
| export const DEFAULT_DERIVATION_PATH = "44'/501'" | |||
There was a problem hiding this comment.
We should be more explicit here and define the full path, not just the prefix and counting on implicit default by Ledger.
| export const DEFAULT_DERIVATION_PATH = "44'/501'" | |
| export const DEFAULT_BIP44_DERIVATION_PATH = "44'/501'/0'/0/0" |
| get publicKey() { | ||
| return this.wallet.payer.publicKey | ||
| } |
There was a problem hiding this comment.
Consider removing this function as payer().pubKey is always available.
| wallet: Solana | ||
| path: string | ||
|
|
||
| constructor(solanaLW: Solana, pubKey: PublicKey, path: string) { |
There was a problem hiding this comment.
By manually allowing to specify both path and pubKey we introduced a way to misconfigure the class and break invariant.
Let's avoid specifying the pubKey for LedgerWallet and get it every time directly from Ledger.
There was a problem hiding this comment.
There could be some problems if constructing the class directly, but we want that is only constructible calling SolanaWallet.create()
I'll make the constructor private to avoid any issue
| } | ||
|
|
||
| get payer(): Keypair { | ||
| throw new Error('Payer method not available on Ledger') |
There was a problem hiding this comment.
If Keypair can be constructed with just the pubkey, consider returning it like that.
There was a problem hiding this comment.
Keypair is the pubkey + priv key together, which we don't have when using Ledger
| const path = c.flags.ledgerPath || DEFAULT_DERIVATION_PATH | ||
| c.wallet = await LedgerWallet.create(path) | ||
| console.info(`Operator address is ${c.wallet.publicKey}`) | ||
| return next() |
There was a problem hiding this comment.
This could be a good place to disconnect the wallet after next() executes, and before this middleware returns. But not 100% sure if that is the workflow of how middlewares are executed...
There was a problem hiding this comment.
This is run just before the execution. Ideally we'd want it after it
* multisig command export and execute options * option to provide different signer than default * comments and improvements * proposal check and inspection command * fixes and improvements * Fixed execution signer. Improvements * Local and Ledger wallet support (#167) * added local and ledger wallet support * Commands inherits multiig wallet * refactor and improvements * More IDL wrappings and improvements * Improved logging information * Small improvements on msig commands * updated serum version * Fix object equal comparison * improved size on msig creation * recent blockhash from confirmed block * naming on multisig commands Co-authored-by: Kristijan Rebernisak <[email protected]>
No description provided.