fix(sns): store generic nervous system function call reply on the proposal - #11453
Rachit2323 wants to merge 4 commits into
Conversation
basvandijk
added
the
security-review-passed
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
|
✅ No security or compliance issues detected. Reviewed everything up to dffffe8. Security OverviewDetected Code Changes
|
|
Re: the governance checklist —
@dfinity/governance-team — flagging for a look since this checklist review is what's currently blocking merge ( Generated by Claude Code |
|
Hi, Rachit2323. Sorry it has taken me a while to look at this. I just came back from a 2 week vacation. |
|
1.
@Rachit2323, could you push those two one-character fixes? That should get 2. Generated by Claude Code |
| /// completed successfully at the IC call level. SNS does not know this | ||
| /// reply's Candid schema, so it is stored as-is (opaque), truncated to at |
There was a problem hiding this comment.
I don't think it needs to be explained why the type here is blob. I mean, mentioning it is not harmful per se, so if you really like it, keep it.
The more interesting fact is that it gets truncated.
The reason for trunctation also does not seem like it needs to be mentioned, but it's not harmful either. I mean, I think the reason can be "very easily" inferred. Everyone knows that space is a finite resource.
Ditto elsewhere, ofc.
| target_canister_id: Some(TARGET_CANISTER_ID.get()), | ||
| target_method_name: Some(TARGET_METHOD.to_string()), | ||
| validator_canister_id: Some(TARGET_CANISTER_ID.get()), | ||
| validator_method_name: Some(TARGET_METHOD.to_string()), |
There was a problem hiding this comment.
For realism, this should be different from target_method_name.
There was a problem hiding this comment.
changed it to format!("validate_{}", target_method) so the validator method is different
| self.perform_execute_generic_nervous_system_function(call) | ||
| .await | ||
| } | ||
| Action::ExecuteGenericNervousSystemFunction(call) => self |
There was a problem hiding this comment.
What happened to braces? Other arms have them. Did rustfmt force you to get rid of them? If not, please, make this code like the rest.
There was a problem hiding this comment.
Added the braces back to match the other arms — rustfmt hadn't removed them.
| topic: Some(i32::from(proposal_topic)), | ||
|
|
||
| // A new proposal has not been executed yet, so there is no reply. | ||
| execution_reply: ProposalData::default().execution_reply, |
There was a problem hiding this comment.
This seems like an extravagant way to obtain a None.
There was a problem hiding this comment.
just changed it to None.
| id: u64, | ||
| target_canister_id: CanisterId, | ||
| target_method: &str, |
There was a problem hiding this comment.
I don't think any callers care to pick these, so just use fixed values + lazy_static.
There was a problem hiding this comment.
dropped the params and used fixed values with lazy_static.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The reversed timeout predicate can make asynchronous proposal execution tests fail immediately.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Stores bounded raw replies from successful generic SNS function calls on proposals for later inspection.
Changes:
- Captures, truncates, and persists target-canister replies.
- Exposes replies through protobuf and Candid APIs.
- Adds reply propagation, failure, and truncation tests.
| File | Description |
|---|---|
rs/sns/governance/unreleased_changelog.md |
Documents reply persistence. |
rs/sns/governance/src/proposal.rs |
Handles reply initialization and limited proposal views. |
rs/sns/governance/src/pb/conversions.rs |
Converts the new API field. |
rs/sns/governance/src/governance/test_helpers.rs |
Adds a shared execution helper, but its timeout predicate is reversed. |
rs/sns/governance/src/governance/execute_generic_nervous_system_function_tests.rs |
Tests reply persistence and truncation. |
rs/sns/governance/src/governance/assorted_governance_tests.rs |
Uses the shared execution helper. |
rs/sns/governance/src/governance.rs |
Stores bounded replies during execution. |
rs/sns/governance/src/gen/ic_sns_governance.pb.v1.rs |
Adds the generated storage field. |
rs/sns/governance/src/canister_control.rs |
Returns opaque call replies. |
rs/sns/governance/src/canister_control_tests.rs |
Tests reply and error propagation. |
rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto |
Defines the persisted reply field. |
rs/sns/governance/canister/governance.did |
Exposes replies through Candid. |
rs/sns/governance/api/src/ic_sns_governance.pb.v1.rs |
Adds the public API field. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // less than 1 s (on my Macbook Pro 2019 Intel). The reason for this | ||
| // generous limit is twofold: 1. avoid flakes in CI, while at the same | ||
| // time 2. do not run forever if something goes wrong. | ||
| let give_up = || now() < start + std::time::Duration::from_secs(30); |
There was a problem hiding this comment.
Confirmed — this is a real bug. give_up = || now() < start + Duration::from_secs(30) returns true for the whole first 30 seconds (so a proposal that isn't final on the very first poll panics immediately) and false forever after 30s elapses (so a genuinely stuck proposal never actually triggers the "took too long" panic). The comparison is inverted.
Fix: let give_up = || now() > start + std::time::Duration::from_secs(30);
@Rachit2323, could you fix this in your next push along with the clippy fix above?
Generated by Claude Code
There was a problem hiding this comment.
flipped it to now() >= start + 30s .
|
Hmm. The size thing is worrying. Based on the comment, I don't think we can increase the limit. We need to find way(s) to put this WASM on a diet. |
|
@daniel-wong-dfinity-org-twin For the size issue: I can switch just this canister to opt-level "z" (optimizes for size), which should save way more than the 2 KB we're over. Downside is it runs a little slower. Want me to do that, or would you rather bump the limit? |
What was wrong: when the SNS asks another canister to do something, that canister sends back an answer. But we were never actually reading that answer, we just checked "did it respond at all" and then threw the answer away. So even if the answer said "this didn't work," we'd still mark it as a success, and nobody could ever go back and see what really happened, because we deleted it.
What we fixed: we stopped deleting the answer. Now we save it, so anyone can look at it later and see exactly what came back.
What we did NOT fix: we still can't automatically tell if the answer means success or failure, because this feature can talk to any canister, and every one of them answers differently. There's no way to understand all of them automatically. So we're not trying to guess anymore, we're just making sure the answer doesn't disappear.
Small limit: if the answer is really long, we only keep part of it, so it doesn't take up too much space.
Testing: added tests to check the answer gets saved properly, that nothing changed for the failure case, and that long answers get shortened correctly.