-
Notifications
You must be signed in to change notification settings - Fork 44
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
Multisig support #37
Multisig support #37
Conversation
@@ -37,36 +39,90 @@ export default class SetBilling extends SolanaCommand { | |||
this.requireFlag('state', 'Provide a valid state address') | |||
} | |||
|
|||
execute = async () => { | |||
makeRawTransaction = async (): Promise<SolanaRawTransaction> => { |
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.
This probably should return a set of txs. There could be cases where many tx are needed to perform the operation (see writeOffchainConfig
)
7f8c0fa
to
607762e
Compare
gauntlet/packages/gauntlet-solana-contracts/src/commands/contracts/multisig/create.ts
Outdated
Show resolved
Hide resolved
|
||
this.program = this.loadProgram(this.multisig.idl, this.address) | ||
const txResults = [] | ||
if (!this.flags.tx) { |
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 think we should stick to the 3 steps: create, approve and execute, and perform just one action per execution.
- Create: Should create the tx proposal, and outputs the the tx public key for future actions.
- Approve: Approves (we can use the
--approve
flag) the proposal created before. We should provide the tx public key with the proposal flag (--proposal
) - Executes: Executes the proposal, if the approval is ready
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.
Adding the single --proposal
flag flow as discussed.
gauntlet/packages/gauntlet-solana-contracts/src/commands/contracts/multisig/setThreshold.ts
Outdated
Show resolved
Hide resolved
gauntlet/packages/gauntlet-serum-multisig/src/commands/multisig.ts
Outdated
Show resolved
Hide resolved
Current flow: Run the command using Creation, approval, execution happens automatically if the proposal is eligible. If not, user is prompted to add a |
… to multisigSigner
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.
It looks so much better! Just needs some final changes
proposal: PublicKey | ||
multisigAddress: PublicKey | ||
multisigSigner: PublicKey | ||
owners: [PublicKey] | ||
threshold: number |
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.
Let's try to reduce the amount of context variables. I'd only leave command
, program
and multisigAddress
this.require(process.env.MULTISIG_ADDRESS != null, 'Please set MULTISIG_ADDRESS env var') | ||
this.multisigAddress = new PublicKey(process.env.MULTISIG_ADDRESS || '') |
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.
Move this into the constructor.
this.require(process.env.MULTISIG_ADDRESS != null, 'Please set MULTISIG_ADDRESS env var') | |
this.multisigAddress = new PublicKey(process.env.MULTISIG_ADDRESS || '') | |
this.require(!!process.env.MULTISIG_ADDRESS, 'Please set MULTISIG_ADDRESS env var') | |
this.multisigAddress = new PublicKey(process.env.MULTISIG_ADDRESS) |
this.multisigAddress = new PublicKey(process.env.MULTISIG_ADDRESS || '') | ||
const multisig = getContract(CONTRACT_LIST.MULTISIG, '') | ||
this.program = this.loadProgram(multisig.idl, multisig.programId.toString()) | ||
logger.info(`Multisig Address: ${process.env.MULTISIG_ADDRESS}`) |
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.
We should already have defined this.multisigAddress
here, no need to use process.env.MULTISIG_ADDRESS
wrapAction = async (action) => { | ||
try { | ||
const tx = await action |
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.
action
seems like it's being executed already. Try to wrap the function first, then add the args you want for this new fn
wrapAction = async (action) => { | |
try { | |
const tx = await action | |
type ProposalAction = (rawTx: RawTransaction) => Promise<string> | |
... | |
wrapAction = (proposalAction: ProposalAction) => (rawTx: RawTransaction) { | |
try { | |
const tx = await proposalAction(rawTx) |
You would consume it like:
this.wrapAction(this.createProposal)(rawTxs[0])
wrapAction = async (action) => { | ||
try { | ||
const tx = await action | ||
this.inspectProposalState(this.flags.proposal) |
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.
You should not use this.flags.proposal
here anymore. There should be already a validated proposal
, that we receive as an argument
const tx = await this.program.rpc.createTransaction(rawTx.programId, rawTx.accounts, rawTx.data, { | ||
accounts: { | ||
multisig: this.multisigAddress, | ||
transaction: this.flags.proposal, |
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 here, no flags
here anymore
const tx = await this.program.rpc.approve({ | ||
accounts: { | ||
multisig: this.multisigAddress, | ||
transaction: this.flags.proposal, |
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 here
return tx | ||
} | ||
|
||
remainingApprovalsNeeded = (proposalState, threshold: number): number => { |
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.
This fn should return already a boolean indicating if it is ready for execution
refactor
this.require(!!process.env.MULTISIG_ADDRESS, 'Please set MULTISIG_ADDRESS env var') | ||
this.multisigAddress = new PublicKey(process.env.MULTISIG_ADDRESS) | ||
const multisig = getContract(CONTRACT_LIST.MULTISIG, '') | ||
this.program = this.loadProgram(multisig.idl, multisig.programId.toString()) |
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.
@RodrigoAD I had to move these in execute() again because I was getting errors.
const input = this.makeInput(this.flags.input) | ||
|
||
const input = this.makeInput( | ||
({ |
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.
Forgot to remove this. It should be: const input = this.makeInput(this.flags.input)
https://github.com/project-serum/multisig/releases/tag/v0.7.0 for artifact