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

DPA-1413: fix(solana): IsOperation in timelock inspector #231

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

graham-chainlink
Copy link
Contributor

@graham-chainlink graham-chainlink commented Jan 10, 2025

Implement status operation check for timelock inspector for solana.

e2e tests will come in another PR as that involves more setup and work.

JIRA: https://smartcontract-it.atlassian.net/browse/DPA-1413


Below is a summarization created by an LLM (gpt-4o-2024-08-06). Be mindful of hallucinations and verify accuracy.

Why

Introduces a new TimelockInspector for Solana chains to access the RBACTimelock contract, providing functionalities to check operation status and manage timelock operations effectively.

What

  • timelock_inspector.go
    • Added TimelockInspector struct and methods for Solana chains
    • Implemented IsOperation to check operation existence
    • Implemented IsOperationPending to check pending status
    • Implemented IsOperationReady to check readiness status
    • Implemented IsOperationDone to check completion status
    • Added helper function getOpData to retrieve operation data
    • Added safeConvertUint64ToInt64 for safe type conversion

Copy link

changeset-bot bot commented Jan 10, 2025

🦋 Changeset detected

Latest commit: 6138e73

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

@graham-chainlink graham-chainlink changed the title fix(solana): IsOperation in timelock inspector [DRAFT] fix(solana): IsOperation in timelock inspector Jan 10, 2025
return false, err
}

return op.Timestamp > TimelockOpDoneTimestamp && ts <= int64(*blockTime), nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have to perform these check manually because anchor-go does not generate bindings for these.

Reason

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be 100% sure that blockTime != nil when err == nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can add a guard to be safe.

return false, err
}

ts, err := safeConvertUint64ToInt64(op.Timestamp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

rather than converting, could you make TimelockOpDoneTimestamp into a uint64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you mean something like uint64(op.Timestamp) , then lint will complain

sdk/solana/timelock_inspector.go:79:12: G115: integer overflow conversion uint64 -> int64 (gosec)
        i := int64(op.Timestamp)

@graham-chainlink graham-chainlink force-pushed the ggoh/DPA-1421/timelock-inspection-status branch from 9dc4f67 to 0ccaaf0 Compare January 13, 2025 05:12
@graham-chainlink graham-chainlink changed the title [DRAFT] fix(solana): IsOperation in timelock inspector DPA-1413: fix(solana): IsOperation in timelock inspector Jan 13, 2025
@graham-chainlink graham-chainlink force-pushed the ggoh/DPA-1421/timelock-inspection-status branch 2 times, most recently from f465bd3 to a388943 Compare January 13, 2025 05:15
@graham-chainlink graham-chainlink marked this pull request as ready for review January 13, 2025 05:16
@graham-chainlink graham-chainlink requested a review from a team as a code owner January 13, 2025 05:16
@graham-chainlink graham-chainlink force-pushed the ggoh/DPA-1421/timelock-inspection-status branch from a388943 to c5e507f Compare January 13, 2025 05:25

mcm.SetProgramID(programID)

pda, err := FindTimelockOperationPDA(programID, seed)
Copy link
Contributor

Choose a reason for hiding this comment

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

As I said in the slack thread, I think this is missing the operation ID:

func GetOperationPDA(timelockID [32]byte, opID [32]byte) solana.PublicKey {
	pda, _, _ := solana.FindProgramAddress([][]byte{
		[]byte("timelock_operation"),
		timelockID[:],
		opID[:],
	}, config.TimelockProgram)
	return pda
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright updated! Thanks!

@graham-chainlink graham-chainlink force-pushed the ggoh/DPA-1421/timelock-inspection-status branch 3 times, most recently from 8391d70 to ba205f5 Compare January 14, 2025 04:00
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.

Great unit tests, btw. Let's just not forget to add proper e2e tests for the timelock inspector as well. If there's no ticket for it, I think we should create one.

@graham-chainlink
Copy link
Contributor Author

LGTM.

Great unit tests, btw. Let's just not forget to add proper e2e tests for the timelock inspector as well. If there's no ticket for it, I think we should create one.

@gustavogama-cll
yeh that is the next one, i wanted it separate because i needed to update the go generate script to pull in the timelock worker files.

Implement status operation check for timelock inspector for solana.
e2e test will come in another PR as that involves more setup and work.

JIRA: https://smartcontract-it.atlassian.net/browse/DPA-1413
@graham-chainlink graham-chainlink added this pull request to the merge queue Jan 14, 2025
Merged via the queue into main with commit a8447e1 Jan 14, 2025
7 checks passed
@graham-chainlink graham-chainlink deleted the ggoh/DPA-1421/timelock-inspection-status branch January 14, 2025 06:27
This was referenced Jan 14, 2025
akhilchainani pushed a commit that referenced this pull request Jan 28, 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

- [#256](#256)
[`45c6a2e`](45c6a2e)
Thanks [@akhilchainani](https://github.com/akhilchainani)! - Allow
callProxy execute capability

- [#245](#245)
[`7a5944e`](7a5944e)
Thanks [@gustavogama-cll](https://github.com/gustavogama-cll)! - Add
context and Converter map TimelockConverter.Convert params

- [#231](#231)
[`a8447e1`](a8447e1)
Thanks [@graham-chainlink](https://github.com/graham-chainlink)! -
feat(solana): timelock inspection - operation statuses check

- [#242](#242)
[`c610826`](c610826)
Thanks [@gustavogama-cll](https://github.com/gustavogama-cll)! - Add
TimelockProposal.Convert for solana

- [#238](#238)
[`abde70c`](abde70c)
Thanks [@graham-chainlink](https://github.com/graham-chainlink)! -
feat(solana): implement get roles for timelock inspection

- [#236](#236)
[`150a1f6`](150a1f6)
Thanks [@graham-chainlink](https://github.com/graham-chainlink)! - Use
string for inspector return type

- [#209](#209)
[`a71dd79`](a71dd79)
Thanks [@gustavogama-cll](https://github.com/gustavogama-cll)! - Add the
`Configurer` component and `SetConfig` call to the Solana SDK.

- [#223](#223)
[`4adb968`](4adb968)
Thanks [@gustavogama-cll](https://github.com/gustavogama-cll)! - Add a
"context" parameter to all APIs that interact with a blockchain.

- [#227](#227)
[`21d8809`](21d8809)
Thanks [@ecPablo](https://github.com/ecPablo)! - Adds Execute
functionality to solana SDK

- [#248](#248)
[`e153c75`](e153c75)
Thanks [@ecPablo](https://github.com/ecPablo)! - Timelock execute batch
on solana SDK.

- [#211](#211)
[`be76399`](be76399)
Thanks [@graham-chainlink](https://github.com/graham-chainlink)! -
feat(solana): support get opdata, root and root metadata

### Patch Changes

- [#215](#215)
[`9f39403`](9f39403)
Thanks [@gustavogama-cll](https://github.com/gustavogama-cll)! - Add the
`Executor` component and `SetRoot` call to the Solana SDK.

- [#259](#259)
[`a4bc13b`](a4bc13b)
Thanks [@anirudhwarrier](https://github.com/anirudhwarrier)! -
usbwallet: fix ledger access for latest firmware and add Ledger Flex

- [#225](#225)
[`7c9cd3d`](7c9cd3d)
Thanks [@gustavogama-cll](https://github.com/gustavogama-cll)! - Add PDA
finders and ContractAddress parser to the Solana SDK

- [#258](#258)
[`a11a0ee`](a11a0ee)
Thanks [@akhilchainani](https://github.com/akhilchainani)! -
non-breaking change to allow a salt override to proposals

- [#228](#228)
[`b953973`](b953973)
Thanks [@graham-chainlink](https://github.com/graham-chainlink)! -
fix(solana): setProgramID on inspection methods

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