Port CLI to mcms for non CLD integrations #182
Conversation
|
akhilchainani
changed the title
|
| @@ -22,6 +22,16 @@ import ( | |||
|
|
|||
| const SignMsgABI = `[{"type":"bytes32"},{"type":"uint32"}]` | |||
|
|
|||
| // ProposalInterface is the interface for all MCMS proposals. | |||
There was a problem hiding this comment.
Do we want the word Interface in the type? I see we already have Proposal as a struct implementing this interface but I was wondering if we could rename it.
I understand this would be a breaking change, but since we don't have any official clients it wouldn't affect anyone.
There was a problem hiding this comment.
Not sure how others feel about it but, maybe we could rename the current Proposal struct to MCMSProposal.
There was a problem hiding this comment.
+1 on that, I actually were a bit confused how proposal is different from timelockproposal as we additionaly have baseproposal. MCMSProposal seems more straightforward.
| // and merkle tree. | ||
| func NewSignable( | ||
| proposal *Proposal, | ||
| Proposal *Proposal, |
There was a problem hiding this comment.
capitalized method parameters are not very common. Was this the result of an accidental search+replace?
| @@ -75,32 +75,32 @@ func (s *Signable) Sign(signer signer) (sig types.Signature, err error) { | |||
| return types.NewSignatureFromBytes(sigB) | |||
| } | |||
|
|
|||
| // SignAndAppend signs the proposal using the provided signer and appends the resulting signature | |||
| // to the proposal's list of signatures. | |||
| // SignAndAppend signs the Proposal using the provided signer and appends the resulting signature | |||
There was a problem hiding this comment.
seems we have a few accidental renames in the comments as well
| @@ -9,13 +9,15 @@ sonar.exclusions=\ | |||
| **/generate,\ | |||
| **/*.md, \ | |||
| **/mocks/**/* | |||
| cmd/**/* | |||
There was a problem hiding this comment.
I understand excluding the cmd/ files from the coverage analysis (below), but why from the static analysis?
There was a problem hiding this comment.
most of the code under cmd/ was copied from chainlink-deployments/cmd/, correct?
| @@ -93,6 +93,7 @@ issues: | |||
| # Exclude the directory from linting because it will soon be removed and only contains | |||
| # generated code. Can be removed once the gethwrappers directory is removed. | |||
| - sdk/evm/bindings | |||
| - cmd | |||
There was a problem hiding this comment.
We should lint this code, or if there is a linter that is specifically not needed, we can exclude it explicitly.
I am very wary of blanket exclusions
| fmt.Printf("Checking quorum for proposal %s\n", proposalPath) | ||
| proposal, err := loadProposal(proposalPath) | ||
| if err != nil { | ||
| fmt.Printf("Error loading proposal: %s\n", err) |
There was a problem hiding this comment.
This is probably bringing up a linting error, but you can you the print function on the cmd object instead of fmt
Add the following changes:
WriteProposalfunctions to be member functions of theMCMSProposalandTimelockProposaltypesProposalObject in theSignableandExecutablestructsMCMSProposalandTimelockProposalsuch that we can interact with the proposals in a proposal-agnostic wayNote: The CLI component right now is fairly coupled to EVM. This is tech debt that needs to be resolved at a later point in time, as this is being implemented to unblock Keystone efforts, which are currently only targeting EVM chains
We should consolidate this with CLD CLI at some point but I think we can do that in a followup PR