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

fix: update timelock conversion logic to manage predecessors per chain instead of globally #279

Merged
merged 9 commits into from
Feb 3, 2025

Conversation

ecPablo
Copy link
Collaborator

@ecPablo ecPablo commented Feb 3, 2025

This pull request includes significant updates and additions to the timelock_executable.go and related test files to enhance functionality and improve test coverage. The key changes include the introduction of new methods for operation ID retrieval and chain-specific indexing, as well as updates to existing tests to accommodate these changes.

Enhancements to timelock_executable.go:

  • Added GetOpID method to TimelockExecutable for retrieving operation IDs.
  • Introduced GetChainSpecificIndex method to TimelockExecutable to determine the index of an operation within a specific chain.

Updates to Tests:

  • Modified Test_TimelockConverter to adjust the expected predecessors in assertions. [1] [2] [3]
  • Updated scheduleAndExecuteGrantRolesProposal to use the new GetOpID method and validate operation completion. [1] [2]
  • Added a new test Test_TimelockExecutable_GetChainSpecificIndex to verify the chain-specific indexing functionality.

Improvements to timelock_proposal.go:

  • Refactored Convert method in TimelockProposal to streamline predecessor handling and chain metadata conversion. [1] [2]

Test Enhancements:

  • Expanded Test_TimelockProposal_Convert to include additional chain selectors and transactions, ensuring comprehensive validation of the conversion process. [1] [2] [3]

…n selector instead of a global list of predecessors.
@ecPablo ecPablo requested a review from a team as a code owner February 3, 2025 14:32
Copy link

changeset-bot bot commented Feb 3, 2025

🦋 Changeset detected

Latest commit: 4b63200

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

Copy link
Collaborator

@akhilchainani akhilchainani left a comment

Choose a reason for hiding this comment

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

Needs a changeset but other than that, looks good!

gustavogama-cll
gustavogama-cll previously approved these changes Feb 3, 2025
@ecPablo ecPablo added this pull request to the merge queue Feb 3, 2025
@ecPablo ecPablo removed this pull request from the merge queue due to a manual request Feb 3, 2025
chainMetadata.MCMAddress,
t.proposal.Delay,
t.proposal.Action,
t.predecessors[opIdx],
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: this might panic -- specially problematic since GetOpID is public, for now.

But maybe we'll get rid of this method soon. The PR is approved so, feel free to merge as is if you're not concerned.

@ecPablo ecPablo added this pull request to the merge queue Feb 3, 2025
Merged via the queue into main with commit 3287f3c Feb 3, 2025
8 checks passed
@ecPablo ecPablo deleted the fix-timelock-conversion-predecessors-multichain branch February 3, 2025 20:05
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>
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