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

Jump to bottom

Multisig support for more commands #115

Merged
merged 25 commits into from
Jan 24, 2022
Merged

Multisig support for more commands #115

merged 25 commits into from
Jan 24, 2022

Conversation

ebarakos
Copy link
Contributor

No description provided.

@ebarakos ebarakos changed the title Draft multisig support for setConfig, setPayees, setWriter Draft multisig support for rest of the commands Jan 18, 2022
@ebarakos ebarakos changed the title Draft multisig support for rest of the commands Multisig support for more commands Jan 18, 2022
ebarakos
ebarakos commented Jan 19, 2022
View reviewed changes
gauntlet/packages/gauntlet-solana-contracts/src/commands/contracts/ocr2/setPayees.ts Outdated
Comment on lines 132 to 135
const rawTx = await this.makeRawTransaction(this.wallet.payer.publicKey)
const tx = await makeTx(rawTx)
logger.loading('Sending tx...')
const txhash = await parseContractErrors(this.provider.send(tx, [this.wallet.payer]), this.idl)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RodrigoAD any comments here before applying it to all commands?

RodrigoAD
RodrigoAD reviewed Jan 19, 2022
View reviewed changes
gauntlet/packages/gauntlet-solana-contracts/src/lib/utils.ts Outdated
@@ -7,3 +11,34 @@ export const divideIntoChunks = (arr: Array<any> | Buffer, chunkSize: number): a
}
return chunks
}

export const parseContractErrors = async (sendTx: Promise<String>, idl: Idl): Promise<string> => {
Copy link
Member

Choose a reason for hiding this comment

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

Don't wrap the execution, wrap the function. Same as here (https://github.com/smartcontractkit/chainlink-solana/blob/develop/gauntlet/packages/gauntlet-serum-multisig/src/commands/multisig.ts#L99)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding it to SolanaCommand, since it is a method and requires some configuration from this.provider.

gauntlet/packages/gauntlet-solana-contracts/src/lib/utils.ts Outdated Show resolved Hide resolved
gauntlet/packages/gauntlet-solana-contracts/src/lib/utils.ts Outdated Show resolved Hide resolved
@ebarakos ebarakos marked this pull request as ready for review January 20, 2022 11:30
RodrigoAD
RodrigoAD reviewed Jan 20, 2022
View reviewed changes
Copy link
Member

@RodrigoAD RodrigoAD left a comment

Choose a reason for hiding this comment

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

Just needs a bit of cleaning

gauntlet/packages/gauntlet-solana-contracts/src/commands/contracts/ocr2/offchainConfig/begin.ts Outdated Show resolved Hide resolved
gauntlet/packages/gauntlet-solana-contracts/src/commands/contracts/ocr2/offchainConfig/begin.ts Outdated Show resolved Hide resolved
gauntlet/packages/gauntlet-solana-contracts/src/commands/contracts/ocr2/setPayees.ts Outdated Show resolved Hide resolved
gauntlet/packages/gauntlet-solana-contracts/src/commands/contracts/ocr2/setPayees.ts Outdated Show resolved Hide resolved
gauntlet/packages/gauntlet-solana-contracts/src/commands/contracts/ocr2/setPayees.ts Outdated Show resolved Hide resolved
gauntlet/packages/gauntlet-solana-contracts/src/commands/contracts/ownership/acceptOwnership.ts Outdated Show resolved Hide resolved
gauntlet/packages/gauntlet-solana-contracts/src/commands/contracts/ownership/acceptOwnership.ts Outdated Show resolved Hide resolved
gauntlet/packages/gauntlet-solana-contracts/src/commands/contracts/store/setWriter.ts Outdated Show resolved Hide resolved
gauntlet/packages/gauntlet-solana-contracts/src/lib/utils.ts Outdated Show resolved Hide resolved
gauntlet/packages/gauntlet-serum-multisig/src/commands/multisig.ts
logger.info(`Execution TX hash: ${tx.toString()}`)
return tx
// get the command's starting word to map it to the respective IDL(ocr2, store etc)
const { idl } = getContract(command.id.split(':')[0], '')
Copy link
Member

Choose a reason for hiding this comment

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

This is not good. This could be addressed when we change this.
For the moment, leave it as it was

For the moment let's load the contract and program here too. Open an issue please to circle back here. You can use program.idl then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue here if I remove this, is that errors will not be translated properly for multisig commands, leading to confusion for users.

Maybe open an issue for this as well? Tackling it along with the other by keeping some program/idl state on SolanaCommand could be possible.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, open an issue please. I'd say that the same issue can solve both problems

@ebarakos ebarakos requested a review from RodrigoAD January 20, 2022 12:46
@ebarakos ebarakos mentioned this pull request Jan 20, 2022
Closed
RodrigoAD
RodrigoAD requested changes Jan 21, 2022
View reviewed changes
Copy link
Member

@RodrigoAD RodrigoAD left a comment

Choose a reason for hiding this comment

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

I have some more comments, we can sync offline

gauntlet/packages/gauntlet-solana-contracts/src/commands/contracts/ocr2/setConfig.ts Outdated
@@ -69,21 +68,48 @@ export default class SetConfig extends SolanaCommand {

logger.log('Config information:', input)
await prompt(`Continue setting config on ${state.toString()}?`)
Copy link
Member

Choose a reason for hiding this comment

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

This prompt is in many commands. It shouldn't be at tx creation time, but on execution. Please change it in every command that appears like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing them. The only drawback is that users will not be prompted while using multisig, that's why I had left them.

gauntlet/packages/gauntlet-serum-multisig/src/commands/multisig.ts Outdated Show resolved Hide resolved
RodrigoAD
RodrigoAD reviewed Jan 21, 2022
View reviewed changes
gauntlet/packages/gauntlet-solana-contracts/src/commands/contracts/store/setWriter.ts Outdated
@@ -57,22 +57,51 @@ export default class SetWriter extends SolanaCommand {

console.log(`Setting store writer on Store (${storeState.toString()}) and Feed (${feedState.toString()})`)
Copy link
Member

Choose a reason for hiding this comment

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

Move this to execution as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved, but had to recalculate some of the variables. We might need shared state/variables to be shared across execute/makeRawTransaction or some other way to tackle it.

Might be related to #127, to add more shared state.

@ebarakos ebarakos force-pushed the multisig-ocr-commands branch from c516f42 to e61ee2b Compare January 21, 2022 15:44
@ebarakos ebarakos requested a review from RodrigoAD January 21, 2022 15:45
@ebarakos ebarakos merged commit 2b2a993 into develop Jan 24, 2022
@ebarakos ebarakos deleted the multisig-ocr-commands branch January 24, 2022 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RodrigoAD RodrigoAD RodrigoAD approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ebarakos @RodrigoAD