[wip] feat: proposal analyzer skeleton#702
Conversation
|
| ) | ||
|
|
||
| type analyzerEngine struct { | ||
| proposalAnalyzers []analyzer.ProposalAnalyzer |
There was a problem hiding this comment.
One thing I feel we might need to set on the engine interface is some way to map the operation types to the correct analyzer. Not sure if you have ideas around this. Here's my situation:
As I was working through a concrete example what I found is that I needed a way to map the contract_type+contract_version+function to a concrete analyzer implementation. Otherwise, when we pass in the proposal operations, we'd be trying to apply analyzers to operations that might not be "compatible" with.
Maybe we change this to a map, and let the implementor decide what key to use for mapping this, or we provide some other function to define the mapping
There was a problem hiding this comment.
That's a good point. I was thinking we should just let each analyzer handle this, with the assumption that it'll be easy to query this information.
I wonder if it'll scale, though. We might need to experiment a bit.
engine/cld/mcms/analyzer/types.go
Outdated
| BaseAnalyzer | ||
| // can we merge the contexts? can we replace the ExecutionContext with cldf's Environment? | ||
| // should the "TimelockProposal be a "DecodeProposal" instead? | ||
| Analyze(ctx *context.Context, actx AnalyzerContext, ectx ExecutionContext, proposal *mcms.TimelockProposal) (AnalyzedProposal, error) |
There was a problem hiding this comment.
Context on this comment: I'm trying to implement a specific analyzer that runs on any MCMS.SetConfig contract call and displays a diff of the signers vs the on chain config. Adding some notes as I go through this experience
The first thing I noticed when trying to implement this looking at the signature:
Analyze(ctx *context.Context, actx AnalyzerContext, ectx ExecutionContext, proposal *mcms.TimelockProposal) (AnalyzedProposal, error)Is that I needed the folllowing:
- Some type decoded proposal in go - some easy to access go struct where I can check if
proposal.Operation[i]is a SetConfig call. We have themcm.TimelockProposalbut the operations here just gives us the raw bytes. - Putting myself in the shoes of the analyzer dev - right now I'm not clear with this interface if it's my responsibility to build the mappings between the different operation types and the corresponding analyzers or if the engine would be doing this for me. Or if this is just a "global" analysis function.
- Ideally we also have a mechanism so that
OperationAnalyzeralso provisions the decoded operation along with the "raw" mcms operation.
On Another note, I think it would be valuable to document in the design doc the steps a product developer would need to take to implement a concrete analyzer - for example:
- Identify the contract type and version to analyze
- Identify the function in the contract to analyze
- Implement the OperationAnalyzer interface to build a concrete type to analyze the function call.
- Register the analyzer in the engine.
- Run Engine
There was a problem hiding this comment.
Thanks for the comment.
I'm trending torwards your idea that the proposal decoder should indeed be a preliminary step. And that maybe we don't even need the original proposal (assuming the decoded types will contain all relevant metadata)
And I agree the design doc should contain concrete examples -- preferably one for each type of analyzer.
engine/cld/mcms/analyzer/types.go
Outdated
|
|
||
| type AnalyzerContext interface{} | ||
|
|
||
| type ExecutionContext interface{} // domain, environment, etc. |
There was a problem hiding this comment.
As I was implementing a concrete analyzer, here's the things that I ended up needing:
- Blockchains object (ideally pre loaded for me by the engine to the correct domain)
- Datastore to access the contracts and contract types.
- Decoded proposal object to have an easy to access go struct to distinguish what are the function calls in the proposal and the specific param values, etc.
|
I added some comments above, for context, I'm trying to implement a small analyzer for the SetConfig mcms call to add annotations of the new/removed signers. Just listed some of the things that would have helped me implement this. I'll create a PR with the quick & ugly implementation to further discuss but the above comments are the main findings |
| type ProposalAnalyzer interface { | ||
| BaseAnalyzer | ||
| Analyze(ctx context.Context, actx AnalyzerContext, ectx ExecutionContext, proposal DecodedTimelockProposal) (Annotations, error) | ||
| } | ||
|
|
||
| type BatchOperationAnalyzer interface { | ||
| BaseAnalyzer | ||
| Analyze(ctx context.Context, actx AnalyzerContext, ectx ExecutionContext, operation DecodedBatchOperation) (Annotations, error) | ||
| } | ||
|
|
||
| type CallAnalyzer interface { | ||
| BaseAnalyzer | ||
| Analyze(ctx context.Context, actx AnalyzerContext, ectx ExecutionContext, call DecodedCall) (Annotations, error) | ||
| } | ||
|
|
||
| type ParameterAnalyzer interface { | ||
| BaseAnalyzer | ||
| Analyze(ctx context.Context, actx AnalyzerContext, ectx ExecutionContext, param DecodedParameter) (Annotations, error) | ||
| } |
There was a problem hiding this comment.
Looking at rddtool-ccip, seems most analyzers are "CallAnalyzers".
Do you think providing the framework (dependencies, proposal/batch scope, etc.) will encourage teams to write more sophisticated analyzers? Or will most stick with simple validators?
There was a problem hiding this comment.
I'm obviously guessing, but I'd say:
- 75% of the analyzers will be Call analyzers
- 20% will be Parameter analyzers whose sole purpose is to add annotations to customize rendering
- 5% will be Proposal analyzers
- 0% will be BatchOperation analyzers
We do know that there's a demand for Proposal-level analyzers -- it's been mentioned by both CCIP as well as the msig team. But I also think they'll be very complex and as a consequence, much rarer.
There was a problem hiding this comment.
if we can find a real world example in rddtool-ccip of a proposal analyzer, that would be great. If not, we can implement something simple, similar (or even simpler) to what the current msig-tools analyzer does.
gustavogama-cll
force-pushed
the
ggama/feat/proposal-analyzer-skeleton
branch
from
e08943b to
dc253d6
Compare
3 participants
No description provided.