refactor: Reimplement display formats with askama templates - #281
Merged
Merged
Conversation
Master moved the ic pin to b4b0230 (via 007c473), so the templates have to absorb the response-type changes that came with it: - NeuronInfo/Neuron gained eight_year_gang_bonus_base_e8s, rendered in min_neuron_info.txt and full_neuron_info.txt and suppressed when zero. - CanisterSettings gained snapshot_visibility. The settings block is now a `canistersettings` macro, since the new CreateCanisterAndInstallCode action renders it too. - proposal::Action gained CreateCanisterAndInstallCode. Its WASM hash is read off the API action rather than the protobuf one the arm otherwise uses, because From<api::CreateCanisterAndInstallCode> cannot rebuild wasm_module and drops the cached hash with it. - NnsFunction::canister_and_function was removed, so master's local lookup table comes along. Resolution is now skipped for non-Candid payloads, which restores master's behavior of hex-printing a payload whose function is unrecognized instead of failing the whole proposal. Also fills in the DeregisterKnownNeuron, BlessAlternativeGuestOsVersion, TakeCanisterSnapshot and LoadCanisterSnapshot arms, which the template was missing entirely.
There was a problem hiding this comment.
🟡 Changes recommended
An invalid Askama match arm blocks compilation, and several templates omit or misrepresent response data.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Replaces ad-hoc response formatting with compile-time Askama templates across ledger, governance, SNS, GTC, and ckBTC responses.
Changes:
- Adds shared formatting filters and response templates.
- Consolidates ICP and ICRC-1 ledger formatting.
- Adds Askama configuration and updates dependencies/MSRV.
File summaries
| File | Description |
|---|---|
Cargo.toml |
Adds Askama and required IC dependency. |
Cargo.lock |
Locks new dependencies. |
askama.toml |
Configures template discovery and whitespace. |
build.rs |
Tracks template changes. |
src/lib/mod.rs |
Routes ledger responses through the consolidated formatter. |
src/lib/format/mod.rs |
Adds shared Askama filters. |
src/lib/format/ledger.rs |
Implements unified ledger rendering. |
src/lib/format/icp_ledger.rs |
Removes legacy ICP formatting. |
src/lib/format/icrc1.rs |
Removes legacy ICRC-1 formatting. |
src/lib/format/ckbtc.rs |
Migrates ckBTC responses to templates. |
src/lib/format/gtc.rs |
Templates claimed-neuron output. |
src/lib/format/nns_governance.rs |
Migrates NNS governance formatting. |
src/lib/format/sns_governance.rs |
Migrates SNS governance formatting. |
src/lib/format/sns_root.rs |
Templates SNS canister summaries. |
src/lib/format/sns_swap.rs |
Templates SNS swap responses. |
src/lib/format/sns_wasm.rs |
Templates deployed-SNS listings. |
src/lib/format/templates/claim_neurons.txt |
Defines claimed-neuron output. |
src/lib/format/templates/ledger/transfer.txt |
Defines successful transfer output. |
src/lib/format/templates/ledger/transfer_err.txt |
Defines ICP transfer errors. |
src/lib/format/templates/ledger/icrc1_transfer_err.txt |
Defines ICRC-1 transfer errors. |
src/lib/format/templates/ledger/balance.txt |
Defines ledger balance output. |
src/lib/format/templates/ckbtc/update_balance.txt |
Defines update-balance results. |
src/lib/format/templates/ckbtc/update_balance_err.txt |
Defines update-balance errors. |
src/lib/format/templates/ckbtc/retrieve_btc.txt |
Defines retrieval-start output. |
src/lib/format/templates/ckbtc/retrieve_btc_status.txt |
Defines retrieval statuses. |
src/lib/format/templates/ckbtc/retrieve_btc_err.txt |
Defines retrieval errors. |
src/lib/format/templates/nns/proposal_info.txt |
Defines NNS proposal details. |
src/lib/format/templates/nns/neuron_ids.txt |
Defines neuron-ID lists. |
src/lib/format/templates/nns/min_neuron_info.txt |
Defines compact neuron details. |
src/lib/format/templates/nns/manage_neuron.txt |
Defines manage-neuron results. |
src/lib/format/templates/nns/list_neurons.txt |
Defines neuron-list layout. |
src/lib/format/templates/nns/full_neuron_info.txt |
Defines full neuron details. |
src/lib/format/templates/nns/claim_or_refresh_neuron_from_account.txt |
Defines staking confirmation. |
src/lib/format/templates/sns/refund.txt |
Defines refund results. |
src/lib/format/templates/sns/refresh_buyer_tokens.txt |
Defines participation balances. |
src/lib/format/templates/sns/new_sale_ticket.txt |
Defines sale-ticket results. |
src/lib/format/templates/sns/manage_neuron.txt |
Defines SNS neuron results. |
src/lib/format/templates/sns/list_snses.txt |
Defines deployed-SNS output. |
src/lib/format/templates/sns/get_buyer_state.txt |
Defines buyer participation output. |
src/lib/format/templates/sns/canisters_summary.txt |
Defines SNS summary layout. |
src/lib/format/templates/sns/canister_summary.txt |
Defines individual canister details. |
Review details
Suppressed comments (4)
src/lib/format/templates/nns/proposal_info.txt:215
MakeProposal (c)is not valid Askama match-arm syntax; tuple payloads requirewith (...). This prevents the template derive from compiling.
src/lib/format/templates/nns/proposal_info.txt:36- A proposal that sets only
wasm_memory_thresholdis treated as non-empty byno_canister_settings, but this macro renders no value for it. Such a proposal therefore displays only the update heading and hides the actual change.
src/lib/format/templates/nns/proposal_info.txt:235 - An
Accountmay be present without an owner. This branch then callsicrc1_helper, whoseaccount.owner.unwrap()panics; the previous formatter emittedunknown accountfor this valid missing-field case.
src/lib/format/templates/sns/canister_summary.txt:15 memory_allocationis measured in bytes, not percent. The current output presents a byte quantity as a percentage, whilecompute_allocationis the percentage-valued setting.
- Files reviewed: 40/41 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
adamspofford-dfinity
marked this pull request as ready for review
September 15, 2026 17:28
|
✅ No security or compliance issues detected. Reviewed everything up to f7118d7. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. |
lwshang
approved these changes
Sep 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rich response display formats are currently implemented with a lot of ad-hoc printf style formatting. This PR replaces it with a proper template system. Askama was chosen because it is checked at compile time, so it will not increase the required test coverage.