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

feat: construct queued proposals with predecessors #276

Merged
merged 16 commits into from
Feb 3, 2025

Conversation

akhilchainani
Copy link
Collaborator

@akhilchainani akhilchainani commented Jan 31, 2025

Enables users to construct proposals given a list of preceding, non-executed proposals, such that proposal signing can happen in parallel assuming a pre-determined sequential execution order. The key changes are as follows:

Enhancements to Proposal Creation:

  • Updated the NewProposal and NewTimelockProposal functions to accept a list of predecessor proposals, allowing for the configuration of operation counts based on the execution order of proposals. (proposal.go, timelock_proposal.go) [1] [2]

Documentation Updates:

  • Added a new section in docs/usage/building-proposals.md to explain how to build proposals with staged but non-executed predecessor proposals.

Code Refactoring:

  • Modified various test files to accommodate the new NewProposal and NewTimelockProposal function signatures that include predecessor proposals. (ledger_test.go, signing.go, proposal_test.go, timelock_proposal_test.go) [1] [2] [3] [4]

New Methods:

  • Added TransactionCounts, ChainMetadatas, and SetChainMetadata methods to the ProposalInterface to manage chain metadata more effectively. (proposal.go, timelock_proposal.go) [1] [2]

Minor Changes:

  • Updated the .changeset/shaggy-pianos-remember.md file to reflect the minor version update for the @smartcontractkit/mcms package.

Copy link

changeset-bot bot commented Jan 31, 2025

🦋 Changeset detected

Latest commit: 44ed3ec

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@smartcontractkit/mcms Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@akhilchainani akhilchainani marked this pull request as ready for review January 31, 2025 22:50
@akhilchainani akhilchainani requested a review from a team as a code owner January 31, 2025 22:50
@akhilchainani akhilchainani changed the title proposal queuing init feat: construct queued proposals with predecessors Jan 31, 2025
Copy link
Collaborator

@ecPablo ecPablo left a comment

Choose a reason for hiding this comment

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

Looking good! I think it might be worth to add and e2e test for this one since it's a slightly complex scenario that we want to test against an in memory chain just to ensure the op count is being managed correctly. It would also help to add a usage on docs, something like "Chaining Proposals Together".

Copy link
Contributor

@gustavogama-cll gustavogama-cll left a comment

Choose a reason for hiding this comment

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

Could you please provide a description in the PR? 🙏
This seems like a non-trivial patch which warrants a bit more context. It's for our own future selves' sake. 😬

proposal.go Outdated Show resolved Hide resolved
utils.go Outdated Show resolved Hide resolved
utils.go Outdated Show resolved Hide resolved
utils.go Outdated Show resolved Hide resolved
utils.go Outdated Show resolved Hide resolved
@@ -41,7 +44,7 @@ func LoadProposal(proposalType types.ProposalKind, filePath string) (ProposalInt
// Ensure the file is closed when done
defer file.Close()

return NewProposal(file)
return NewProposal(file, []io.Reader{}) // TODO: inject predecessors
Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot to ask: is this a blocker?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh shoot not a blocker but that was a miss

Copy link
Contributor

@gustavogama-cll gustavogama-cll left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll second Pablo's request for an e2e test. Would be good to have a ticket for it in our tech-debt epic?

@akhilchainani akhilchainani added this pull request to the merge queue Feb 3, 2025
Merged via the queue into main with commit 27b77d5 Feb 3, 2025
8 checks passed
@akhilchainani akhilchainani deleted the feat/proposal-queuing branch February 3, 2025 19:26
github-merge-queue bot pushed a commit that referenced this pull request Feb 3, 2025
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @smartcontractkit/[email protected]

### Minor Changes

- [#276](#276)
[`27b77d5`](27b77d5)
Thanks [@akhilchainani](https://github.com/akhilchainani)! - Update
constructors to add predecessor proposals for queuing

- [#254](#254)
[`aad56bd`](aad56bd)
Thanks [@gustavogama-cll](https://github.com/gustavogama-cll)! -
feat(solana): add setRoot simulator

- [#279](#279)
[`3287f3c`](3287f3c)
Thanks [@ecPablo](https://github.com/ecPablo)! - Fix bug with multichain
timelock execution with predecessors calculation

### Patch Changes

- [#274](#274)
[`28d52c3`](28d52c3)
Thanks [@graham-chainlink](https://github.com/graham-chainlink)! -
fix(solana): fix simulator side effect bug

Co-authored-by: app-token-issuer-engops[bot] <144731339+app-token-issuer-engops[bot]@users.noreply.github.com>
@akhilchainani
Copy link
Collaborator Author

LGTM, but I'll second Pablo's request for an e2e test. Would be good to have a ticket for it in our tech-debt epic?

Ill add this and that LoadProposal bug in a follow up PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants