feat: decouple rendering from intermediate decoded representation#541
Conversation
…eate base text renderer
🦋 Changeset detectedLatest commit: 1a1dc6e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the analyzer package to decouple rendering logic from data representation and adds support for multiple output formats. The refactoring introduces a cleaner separation of concerns by replacing descriptor-based types with field-based types and moving template rendering logic into dedicated renderer classes.
Key changes:
- Renamed
Descriptortypes toFieldtypes (e.g.,SimpleDescriptor→SimpleField,NamedDescriptor→NamedField) throughout the codebase - Created
TextRendererand enhancedMarkdownRendererwith template-based rendering, moving templates from inline strings to separate.tmplfiles - Added a
--formatflag to theanalyze-proposalCLI command to support both markdown and text output formats
Reviewed Changes
Copilot reviewed 55 out of 55 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
experimental/analyzer/fields.go |
Defines new field types (FieldValue, NamedField, ArrayField, etc.) replacing the old descriptor types |
experimental/analyzer/renderer_text.go |
Implements new TextRenderer for plain text output format |
experimental/analyzer/renderer_markdown.go |
Refactored to use template files and enhanced field rendering |
experimental/analyzer/templates/text/*.tmpl |
New text format templates for rendering different field types |
experimental/analyzer/templates/markdown/*.tmpl |
New markdown format templates extracted from inline strings |
experimental/analyzer/utils.go |
Updated toNamedFields() and getFieldValue() to use new field types |
experimental/analyzer/evm_tx_decoder.go |
Updated to use FieldAnalyzer and field types instead of descriptors |
experimental/analyzer/proposal_context.go |
Added SetRenderer() method and renamed DescriptorContext() to FieldsContext() |
engine/cld/legacy/cli/mcmsv2/mcms_v2.go |
Added --format flag and createRendererFromFormat() helper function |
| Test files | Updated all test files to use new field types and comparison logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
| return analyzer.NewMarkdownRenderer() | ||
| default: | ||
| // Default to markdown if format is not specified or invalid | ||
| return analyzer.NewMarkdownRenderer() |
There was a problem hiding this comment.
I think we should return an error, rather than silently defaulting to markdown.
There was a problem hiding this comment.
(I'm ok with the default being markdown; it's just the --format html returning markdown that I'm more concerned about)
| @@ -0,0 +1 @@ | |||
| array[{{.Length}}]: {{if eq .Length 0}}[]{{else}}[{{range $i, $elem := .Elements}}{{if $i}}, {{end}}{{compactValue $elem $.Context}}{{end}}{{if gt .Length 3}}, … (+{{sub .Length 3}}){{end}}]{{end}} No newline at end of file | |||
There was a problem hiding this comment.
I think you can add dashes to add new lines to the template file without adding new lines to the output.
There was a problem hiding this comment.
| {{range $i, $op := .Operations}}Operation #{{$i}} | ||
| Chain selector: {{$op.ChainSelector}} ({{$op.ChainName}}) | ||
| {{range $call := $op.Calls}}{{indent (renderCall $call $.Context)}}{{end}} | ||
| {{end}} No newline at end of file |
There was a problem hiding this comment.
nit: some files do not have a final "new line" char, which is bound to cause "noisy" one line diffs whenever another engineer opens them in their IDEs and submits a PR.
There was a problem hiding this comment.
This was on purpose since I wanted the final output to now have extra newlines. But maybe I can make it so that all templates have a new line just to avoid IDE problems
There was a problem hiding this comment.
I think you can use the dashes to avoid those extra newlines.
| return truncateMiddle(strings.ReplaceAll(s, "\n", " "), MaxCompactValueLength) | ||
| return truncateMiddle(fmt.Sprintf("<%s>", f.GetType()), MaxCompactValueLength) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we can leverage reflection to greatly simplify the logic in this file and make the type associations more DRY.
But I'm ok with taking incremental steps, so let's start with the current solution.
|
Now the deed is done but I think this should have been two PRs: one renaming "Descriptor" to "Field", and another with the renderer changes. |
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 ## chainlink-deployments-framework@0.60.0 ### Minor Changes - [#541](#541) [`909e6f4`](909e6f4) Thanks [@ecPablo](https://github.com/ecPablo)! - refactor proposal analyzer to add a text renderer and move templates to separate files - [#535](#535) [`b7c8d06`](b7c8d06) Thanks [@graham-chainlink](https://github.com/graham-chainlink)! - feat(engine): load catalog into datastore ### Patch Changes - [#547](#547) [`2229818`](2229818) Thanks [@jkongie](https://github.com/jkongie)! - Bump chain-selectors to v1.0.77 - [#534](#534) [`9572077`](9572077) Thanks [@graham-chainlink](https://github.com/graham-chainlink)! - fix(catalog): use enum instead of string --------- Co-authored-by: app-token-issuer-engops[bot] <144731339+app-token-issuer-engops[bot]@users.noreply.github.com>
TextRendereras base renderer for the MarkdownRenderer to decouple formatting assumptions from intermediate representations of the decoded proposal.descriptorstofieldsI think the new name is a bit easier to follow.formatflag to theanalyze-proposalcommand so users can outputs either a text or markdown formatAI Summary
This pull request refactors how decoded calls and their arguments are represented and rendered in the analyzer package, and introduces support for specifying output formats in the MCMSv2 CLI. The main focus is on replacing the legacy
Descriptor-based argument types with a more structuredField-based approach, improving type safety and clarity for transaction analysis (especially Aptos transactions). It also adds a mechanism to select between markdown and text output formats for proposal analysis.Analyzer Data Model Refactoring
Replaced usage of
NamedDescriptorandDescriptortypes with newNamedFieldandFieldValuetypes throughout the analyzer codebase, including inDecodedCall, test cases, and Aptos transaction analysis. This improves type safety and enables richer field representations such as arrays and addresses. [1] [2] [3] [4] [5] [6] [7]Updated test assertions to compare field types and values using the new field model, ensuring correctness of the new structure.
Removed the legacy
Describemethod fromDecodedCalland replaced it with a newStringmethod that uses the updated field model. The actual rendering is now intended to be handled by dedicated renderer classes. [1] [2]CLI Output Format Support
Added a
--formatflag to the MCMSv2 CLIanalyze-proposalcommand, allowing users to select between markdown and text output formats for proposal analysis. [1] [2]Implemented the
createRendererFromFormathelper function to instantiate the appropriate renderer based on the requested format.Ensured the selected renderer is set in the proposal context before analysis, enabling flexible output formatting.
Test and Mock Updates
These changes modernize the analyzer's internal data model, improve output flexibility, and lay the groundwork for future enhancements in transaction decoding and rendering.