Skip to content

Commit

Permalink
Merge branch 'jason/NNS1-3045' into 'master'
Browse files Browse the repository at this point in the history
fix(sns): NNS1-3045 Truncate the logo fields in ManageSnsMetadata/ManageLedgerParameters from list_proposals response

Note that we are not truncating within get_propsal, in the same way as the UpgradeSnsControlledCanister and ExecuteGenericNervousSystemFunction payloads ("summarize blob fields" [here](https://sourcegraph.com/github.com/dfinity/ic@29e29f33844c16cdc7a815390ecb48a644b982e4/-/blob/rs/sns/governance/src/types.rs?L1641)). This is because there is no other way to get the logos in the proposal verifiably, where as UpgradeSnsControlledCanister has the wasm hash, while ExecuteGenericNervousSystemFunction has the validator mechanism.

On the other hand, it's not as crucial for get_proposal to return small enough response since it's just for one proposal, and logos are still bounded at \~300KiB.

Closes NNS1-3045 

Closes NNS1-3045

See merge request dfinity-lab/public/ic!19135
  • Loading branch information
jasonz-dfinity committed May 7, 2024
2 parents d63549e + 5416a61 commit 746961a
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 1 deletion.
70 changes: 70 additions & 0 deletions rs/sns/governance/src/proposal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4529,4 +4529,74 @@ Version {
}
);
}

#[test]
fn limited_proposal_data_for_list_proposals_limited_manage_sns_metadata() {
let original_proposal_data = ProposalData {
proposal: Some(Proposal {
action: Some(Action::ManageSnsMetadata(ManageSnsMetadata {
logo: Some("some logo".to_string()),
url: Some("some url".to_string()),
name: Some("some name".to_string()),
description: Some("some description".to_string()),
})),
..Default::default()
}),
..Default::default()
};

let limited_proposal_data =
original_proposal_data.limited_for_list_proposals(&HashSet::new());

assert_eq!(
limited_proposal_data,
ProposalData {
proposal: Some(Proposal {
action: Some(Action::ManageSnsMetadata(ManageSnsMetadata {
logo: None,
url: Some("some url".to_string()),
name: Some("some name".to_string()),
description: Some("some description".to_string()),
},)),
..Default::default()
}),
..Default::default()
}
);
}

#[test]
fn limited_proposal_data_for_list_proposals_limited_manage_ledger_parameters() {
let original_proposal_data = ProposalData {
proposal: Some(Proposal {
action: Some(Action::ManageLedgerParameters(ManageLedgerParameters {
transfer_fee: Some(100),
token_name: Some("some name".to_string()),
token_symbol: Some("some symbol".to_string()),
token_logo: Some("some logo".to_string()),
})),
..Default::default()
}),
..Default::default()
};

let limited_proposal_data =
original_proposal_data.limited_for_list_proposals(&HashSet::new());

assert_eq!(
limited_proposal_data,
ProposalData {
proposal: Some(Proposal {
action: Some(Action::ManageLedgerParameters(ManageLedgerParameters {
transfer_fee: Some(100),
token_name: Some("some name".to_string()),
token_symbol: Some("some symbol".to_string()),
token_logo: None,
},)),
..Default::default()
}),
..Default::default()
}
);
}
}
32 changes: 31 additions & 1 deletion rs/sns/governance/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use crate::{
ClaimSwapNeuronsError, ClaimSwapNeuronsResponse, ClaimedSwapNeuronStatus,
DefaultFollowees, DeregisterDappCanisters, Empty, ExecuteGenericNervousSystemFunction,
GovernanceError, ManageDappCanisterSettings, ManageLedgerParameters,
ManageNeuronResponse, MintSnsTokens, Motion, NervousSystemFunction,
ManageNeuronResponse, ManageSnsMetadata, MintSnsTokens, Motion, NervousSystemFunction,
NervousSystemParameters, Neuron, NeuronId, NeuronPermission, NeuronPermissionList,
NeuronPermissionType, ProposalId, RegisterDappCanisters, RewardEvent,
TransferSnsTreasuryFunds, UpgradeSnsControlledCanister, UpgradeSnsToNextVersion, Vote,
Expand Down Expand Up @@ -1517,6 +1517,12 @@ impl Action {
Action::ExecuteGenericNervousSystemFunction(action) => {
Action::ExecuteGenericNervousSystemFunction(action.limited_for_list_proposals())
}
Action::ManageSnsMetadata(action) => {
Action::ManageSnsMetadata(action.limited_for_list_proposals())
}
Action::ManageLedgerParameters(action) => {
Action::ManageLedgerParameters(action.limited_for_list_proposals())
}
action => action.clone(),
}
}
Expand Down Expand Up @@ -1674,6 +1680,30 @@ impl ExecuteGenericNervousSystemFunction {
}
}

impl ManageSnsMetadata {
/// Returns a clone of self, except that the logo is cleared because it can be large.
pub(crate) fn limited_for_list_proposals(&self) -> Self {
Self {
url: self.url.clone(),
name: self.name.clone(),
description: self.description.clone(),
logo: None,
}
}
}

impl ManageLedgerParameters {
/// Returns a clone of self, except that the logo is cleared because it can be large.
pub(crate) fn limited_for_list_proposals(&self) -> Self {
Self {
transfer_fee: self.transfer_fee,
token_name: self.token_name.clone(),
token_symbol: self.token_symbol.clone(),
token_logo: None,
}
}
}

/// If blob is of length <= 64 (bytes), a copy is returned. Otherwise, a (UTF-8
/// encoded) human-readable textual summary is returned. This summary is
/// guaranteed to be of length > 64. Therefore, it is always possible to
Expand Down

0 comments on commit 746961a

Please sign in to comment.