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.

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 batching #406

Closed
wants to merge 46 commits into from
Closed

Multisig batching #406

wants to merge 46 commits into from

Conversation

archseer
Copy link
Collaborator

@archseer archseer commented Oct 7, 2022

(DO NOT SQUASH)

  • serum_multisig:approve allows approving multiple transactions at once
  • proposeConfig is now a single command, replacing createProposal, proposeConfig, proposeOffchainConfig, proposePayees and finalizeProposal
    • to support this, sendRawTx now uses more reliable send & confirm code that can retry
    • if a proposal fails halfway through:
      • it can be resumed by passing a --proposal <PROPOSAL_ID>
      • if aborting, it should be manually closed to reclaim rent
  • localnet.sh sets up a local node with programs pre-deployed and a pre-funded account
  • token:create_account was changed to allow creating multiple token accounts at once (single tx)
  • setup.dev.flow now actually works again (pending https://github.com/smartcontractkit/gauntlet-core/pull/100)
  • typescript, nodejs and golang were upgraded

@archseer archseer temporarily deployed to integration October 7, 2022 18:01 Inactive
@archseer archseer changed the base branch from develop to audit-fixes October 7, 2022 18:01
Base automatically changed from audit-fixes to develop October 11, 2022 00:15
@archseer archseer temporarily deployed to integration November 17, 2022 05:18 Inactive
@archseer archseer temporarily deployed to integration November 17, 2022 05:18 Inactive
@archseer archseer temporarily deployed to integration November 17, 2022 05:23 Inactive
@archseer archseer temporarily deployed to integration November 17, 2022 05:23 Inactive
@archseer archseer temporarily deployed to integration November 17, 2022 05:23 Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2022

Smoke Test Results

1 tests  ±0   0 ✔️ ±0   6m 55s ⏱️ +11s
1 suites ±0   0 💤 ±0 
1 files   ±0   1 ±0 

For more details on these failures, see this check.

Results for commit 783c334. ± Comparison against base commit 7c09106.

♻️ This comment has been updated with latest results.

@archseer archseer temporarily deployed to integration November 17, 2022 08:01 Inactive
@archseer archseer temporarily deployed to integration November 17, 2022 08:01 Inactive
@archseer archseer temporarily deployed to integration November 17, 2022 08:01 Inactive
import { CONTRACT_LIST, getContract } from '../lib/contracts'

export default class Approve extends SolanaCommand {
static id = 'serum_multisig:approve'
Copy link
Member

Choose a reason for hiding this comment

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

When is this going to be used?

Copy link
Member

Choose a reason for hiding this comment

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

Reopening this, the whole purpose of the multisig command is validation. Here the users will approve proposals they don't know what they do.

If approval batching is required, it should be built within the multisig wrapper, so Gauntlet can make proper validations and inform the users about the actions they'll do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a quick workaround from an immediate need: without batching we need a ledger approval per feed per owner. This at least reduces it to a command (per 6-7 feeds) per owner.

It would be great to fix this in the multisig wrapper, but it's a larger job since we'd need to provide inputs for multiple invocations, verify them then pack that into a single tx. This is slightly tricky UX-wise too, since each command can take additional flags, for example --secret per ocr2:accept_proposal:multisig.

I think the multisig wrapper needs a general re-work to stop being so stateful too. It's very easy to accidentally call the code without a proposal (or worse yet, the code is faulty and fetches no proposal), which ends up creating new proposals rather than approving.

We've already had that happen on starknet: proposalId was missing in the implementation but the command was running successfully (and tests passing), creating new proposals over and over again: smartcontractkit/chainlink-starknet@dacaae3#diff-8257597e02534a1f29c2fcb789a25491779bd7ac33359412616550f5c422d83cR208

@archseer archseer temporarily deployed to integration November 25, 2022 06:35 Inactive
@archseer archseer temporarily deployed to integration November 25, 2022 06:35 Inactive
@archseer archseer temporarily deployed to integration November 25, 2022 06:35 Inactive
@archseer archseer temporarily deployed to integration November 25, 2022 07:12 Inactive
@archseer archseer temporarily deployed to integration November 25, 2022 07:12 Inactive
@archseer archseer temporarily deployed to integration November 25, 2022 07:13 Inactive
import { CONTRACT_LIST, getContract } from '../lib/contracts'

export default class Approve extends SolanaCommand {
static id = 'serum_multisig:approve'
Copy link
Member

Choose a reason for hiding this comment

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

Reopening this, the whole purpose of the multisig command is validation. Here the users will approve proposals they don't know what they do.

If approval batching is required, it should be built within the multisig wrapper, so Gauntlet can make proper validations and inform the users about the actions they'll do.

@archseer archseer temporarily deployed to integration November 29, 2022 11:17 Inactive
@archseer archseer temporarily deployed to integration November 29, 2022 11:17 Inactive
@archseer archseer temporarily deployed to integration November 29, 2022 11:17 Inactive
@archseer archseer temporarily deployed to integration November 30, 2022 04:33 Inactive
@archseer archseer temporarily deployed to integration November 30, 2022 04:33 Inactive
@archseer archseer temporarily deployed to integration November 30, 2022 04:33 Inactive
Copy link
Contributor

github-actions bot commented Dec 3, 2024

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Dec 3, 2024
@github-actions github-actions bot closed this Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants