Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Store information about operators' validator status #37

Merged
merged 4 commits into from
Jan 27, 2022
Merged

Store information about operators' validator status #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48be09b
valset: keep validator status with operator info
uint Jan 25, 2022
919d05e
valset: test active_validator field in operator info
uint Jan 25, 2022
e09bbee
lints
uint Jan 25, 2022
41d7206
valset: optimize marking operators as active val
uint Jan 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 87 additions & 69 deletions contracts/tgrade-valset/src/contract.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ pub fn instantiate(
let info = OperatorInfo {
pubkey,
metadata: op.metadata,
active_validator: false,
};
operators().save(deps.storage, &oper, &info)?;
}
Expand Down Expand Up @@ -203,7 +204,11 @@ fn execute_register_validator_key(
let pubkey: Ed25519Pubkey = pubkey.try_into()?;
let moniker = metadata.moniker.clone();

let operator = OperatorInfo { pubkey, metadata };
let operator = OperatorInfo {
pubkey,
metadata,
active_validator: false,
};
match operators().may_load(deps.storage, &info.sender)? {
Some(_) => return Err(ContractError::OperatorRegistered {}),
None => operators().save(deps.storage, &info.sender, &operator)?,
Expand Down Expand Up @@ -450,6 +455,7 @@ fn list_validator_keys(
metadata: info.metadata,
pubkey: info.pubkey.into(),
jailed_until,
active_validator: info.active_validator,
})
})
.take(limit)
Expand Down Expand Up @@ -570,7 +576,27 @@ fn end_block(deps: DepsMut, env: Env) -> Result<Response, ContractError> {
let old_validators = VALIDATORS.load(deps.storage)?;
VALIDATORS.save(deps.storage, &validators)?;
// determine the diff to send back to tendermint
let (diff, update_members) = calculate_diff(validators, old_validators);
let (diff, add, remove) = calculate_diff(validators, old_validators);
Copy link
Contributor

@maurolacy maurolacy Jan 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, you could use diff directly below, with the convention that zero power means removal.

That way you can keep the calculate_diff signature the same, and keep rewards the distribution computation internal.

let update_members = RewardsDistribution::UpdateMembers {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... Why not just return this as it was before (same function signature for calculate_diff ) and iterate over
for op in update_members.add for example? It should avoid a clone even.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

Copy link
Contributor Author

@uint uint Jan 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd end up in this situation:
A function called calculate_diff actually calculates the rewards distribution msg for use with a specific contract's interface, and then we use that message to get back the diff for another use.

I'd rather make calculate_diff do what it advertises in the first place. Feels less convoluted.

As for the cloning, I think we'd still end up cloning those strings (Addr::unchecked does that when it's constructed from a &str), except they'd be cloned implicitly rather than explicitly. I could be missing something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, i.e. moving the rewards distribution out of calculate_diff is a good thing:tm:. Let's merge as it is.

add: add.clone(),
remove: remove.clone(),
};

// update operators list with info about whether or not they're active validators
for op in add {
operators().update::<_, StdError>(deps.storage, &Addr::unchecked(op.addr), |op| {
let mut op = op.ok_or_else(|| StdError::generic_err("operator doesn't exist"))?;
op.active_validator = true;
Ok(op)
})?;
}
for op in remove {
operators().update::<_, StdError>(deps.storage, &Addr::unchecked(op), |op| {
let mut op = op.ok_or_else(|| StdError::generic_err("operator doesn't exist"))?;
op.active_validator = false;
Ok(op)
})?;
}
Comment on lines +586 to +599
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Consider using diff directly, as mentioned above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, but you need the operator key. Makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative then would be to do this update internally, as part of calculate_diff. But I think this is good / better for clarity.


// Store starting heights of new validators
match &update_members {
Expand Down Expand Up @@ -699,7 +725,7 @@ fn calculate_validators(
fn calculate_diff(
cur_vals: Vec<ValidatorInfo>,
old_vals: Vec<ValidatorInfo>,
) -> (ValidatorDiff, RewardsDistribution) {
) -> (ValidatorDiff, Vec<Member>, Vec<String>) {
// Compute additions and updates
let cur: BTreeSet<_> = cur_vals.iter().collect();
let old: BTreeSet<_> = old_vals.iter().collect();
Expand Down Expand Up @@ -745,10 +771,7 @@ fn calculate_diff(
// Compute, map and append removals to diffs
diffs.extend(removed_diff);

(
ValidatorDiff { diffs },
RewardsDistribution::UpdateMembers { add, remove },
)
(ValidatorDiff { diffs }, add, remove)
}

#[cfg_attr(not(feature = "library"), entry_point)]
Expand Down Expand Up @@ -924,16 +947,14 @@ mod test {
vals
}

fn update_members_msg(remove: Vec<&str>, add: Vec<(&str, u64)>) -> RewardsDistribution {
let remove = remove.into_iter().map(str::to_owned).collect();
let add = add
fn members(members: Vec<(&str, u64)>) -> Vec<Member> {
members
.into_iter()
.map(|(addr, weight)| Member {
addr: addr.to_owned(),
weight,
})
.collect();
RewardsDistribution::UpdateMembers { add, remove }
.collect()
}

// Unit tests for calculate_diff()
Expand Down Expand Up @@ -970,12 +991,12 @@ mod test {
];

// diff with itself must be empty
let (diff, update_members) = calculate_diff(vals.clone(), vals.clone());
let (diff, add, remove) = calculate_diff(vals.clone(), vals.clone());
assert_eq!(diff.diffs, vec![]);
assert_eq!(update_members, update_members_msg(vec![], vec! {}));
assert_eq!((add, remove), (vec![], vec![]));

// diff with empty must be itself (additions)
let (diff, update_members) = calculate_diff(vals.clone(), empty.clone());
let (diff, add, remove) = calculate_diff(vals.clone(), empty.clone());
assert_eq!(
vec![
ValidatorUpdate {
Expand All @@ -989,13 +1010,11 @@ mod test {
],
diff.diffs
);
assert_eq!(
update_members,
update_members_msg(vec![], vec![("op1", 1), ("op2", 2)])
);
assert!(remove.is_empty());
assert_eq!(add, members(vec![("op1", 1), ("op2", 2)]));

// diff between empty and vals must be removals
let (diff, update_members) = calculate_diff(empty, vals.clone());
let (diff, add, remove) = calculate_diff(empty, vals.clone());
assert_eq!(
vec![
ValidatorUpdate {
Expand All @@ -1009,10 +1028,8 @@ mod test {
],
diff.diffs
);
assert_eq!(
update_members,
update_members_msg(vec!["op1", "op2"], vec![])
);
assert!(add.is_empty());
assert_eq!(remove, ["op1", "op2"]);

// Add a new member
let mut cur = vals.clone();
Expand All @@ -1030,56 +1047,59 @@ mod test {
});

// diff must be add last
let (diff, update_members) = calculate_diff(cur, vals.clone());
let (diff, add, remove) = calculate_diff(cur, vals.clone());
assert_eq!(
vec![ValidatorUpdate {
pubkey: Pubkey::Ed25519(b"pubkey3".into()),
power: 3
},],
diff.diffs
);
assert_eq!(update_members, update_members_msg(vec![], vec![("op3", 3)]));
assert!(remove.is_empty());
assert_eq!(add, members(vec![("op3", 3)]));

// add all but (one) last member
let old: Vec<_> = vals.iter().skip(1).cloned().collect();

// diff must be add all but last
let (diff, update_members) = calculate_diff(vals.clone(), old);
let (diff, add, remove) = calculate_diff(vals.clone(), old);
assert_eq!(
vec![ValidatorUpdate {
pubkey: Pubkey::Ed25519(b"pubkey1".into()),
power: 1
},],
diff.diffs
);
assert_eq!(update_members, update_members_msg(vec![], vec![("op1", 1)]));
assert!(remove.is_empty());
assert_eq!(add, members(vec![("op1", 1)]));

// remove last member
let cur: Vec<_> = vals.iter().take(1).cloned().collect();
// diff must be remove last
let (diff, update_members) = calculate_diff(cur, vals.clone());
let (diff, add, remove) = calculate_diff(cur, vals.clone());
assert_eq!(
vec![ValidatorUpdate {
pubkey: Pubkey::Ed25519(b"pubkey2".into()),
power: 0
},],
diff.diffs
);
assert_eq!(update_members, update_members_msg(vec!["op2"], vec![]));
assert!(add.is_empty());
assert_eq!(remove, ["op2"]);

// remove all but last member
let cur: Vec<_> = vals.iter().skip(1).cloned().collect();
// diff must be remove all but last
let (diff, update_members) = calculate_diff(cur, vals);
let (diff, add, remove) = calculate_diff(cur, vals);
assert_eq!(
vec![ValidatorUpdate {
pubkey: Pubkey::Ed25519(b"pubkey1".into()),
power: 0
},],
diff.diffs
);

assert_eq!(update_members, update_members_msg(vec!["op1"], vec![]));
assert!(add.is_empty());
assert_eq!(remove, ["op1"]);
}

// TODO: Another 7 in 1 test to be split
Expand All @@ -1089,12 +1109,13 @@ mod test {
let vals = validators(VALIDATORS);

// diff with itself must be empty
let (diff, update_members) = calculate_diff(vals.clone(), vals.clone());
let (diff, add, remove) = calculate_diff(vals.clone(), vals.clone());
assert_eq!(diff.diffs, vec![]);
assert_eq!(update_members, update_members_msg(vec![], vec![]));
assert!(add.is_empty());
assert!(remove.is_empty());

// diff with empty must be itself (additions)
let (diff, update_members) = calculate_diff(vals.clone(), empty.clone());
let (diff, add, remove) = calculate_diff(vals.clone(), empty.clone());
assert_eq!(
ValidatorDiff {
diffs: vals
Expand All @@ -1107,18 +1128,18 @@ mod test {
},
diff
);
assert!(remove.is_empty());
assert_eq!(
update_members,
update_members_msg(
vec![],
add,
members(
vals.iter()
.map(|vi| (vi.operator.as_str(), vi.power))
.collect()
)
);

// diff between empty and vals must be removals
let (diff, update_members) = calculate_diff(empty, vals.clone());
let (diff, add, remove) = calculate_diff(empty, vals.clone());
assert_eq!(
ValidatorDiff {
diffs: vals
Expand All @@ -1131,16 +1152,19 @@ mod test {
},
diff
);
assert!(add.is_empty());
assert_eq!(
update_members,
update_members_msg(vals.iter().map(|vi| vi.operator.as_str()).collect(), vec![])
remove,
vals.iter()
.map(|vi| vi.operator.as_str())
.collect::<Vec<_>>()
);

// Add a new member
let cur = validators(VALIDATORS + 1);

// diff must be add last
let (diff, update_members) = calculate_diff(cur.clone(), vals.clone());
let (diff, add, remove) = calculate_diff(cur.clone(), vals.clone());
assert_eq!(
ValidatorDiff {
diffs: vec![ValidatorUpdate {
Expand All @@ -1150,22 +1174,20 @@ mod test {
},
diff
);
assert!(remove.is_empty());
assert_eq!(
update_members,
update_members_msg(
vec![],
vec![(
cur.last().as_ref().unwrap().operator.as_str(),
(VALIDATORS + 1) as u64
)]
)
add,
members(vec![(
cur.last().as_ref().unwrap().operator.as_str(),
(VALIDATORS + 1) as u64
)])
);

// add all but (one) last member
let old: Vec<_> = vals.iter().skip(VALIDATORS - 1).cloned().collect();

// diff must be add all but last
let (diff, update_members) = calculate_diff(vals.clone(), old);
let (diff, add, remove) = calculate_diff(vals.clone(), old);
assert_eq!(
ValidatorDiff {
diffs: vals
Expand All @@ -1179,10 +1201,10 @@ mod test {
},
diff
);
assert!(remove.is_empty());
assert_eq!(
update_members,
update_members_msg(
vec![],
add,
members(
vals.iter()
.take(VALIDATORS - 1)
.map(|vi| (vi.operator.as_ref(), vi.power))
Expand All @@ -1193,7 +1215,7 @@ mod test {
// remove last member
let cur: Vec<_> = vals.iter().take(VALIDATORS - 1).cloned().collect();
// diff must be remove last
let (diff, update_members) = calculate_diff(cur, vals.clone());
let (diff, add, remove) = calculate_diff(cur, vals.clone());
assert_eq!(
ValidatorDiff {
diffs: vec![ValidatorUpdate {
Expand All @@ -1203,15 +1225,13 @@ mod test {
},
diff
);
assert_eq!(
update_members,
update_members_msg(vec![vals.last().unwrap().operator.as_ref()], vec![])
);
assert!(add.is_empty());
assert_eq!(remove, vec![vals.last().unwrap().operator.as_ref()]);

// remove all but last member
let cur: Vec<_> = vals.iter().skip(VALIDATORS - 1).cloned().collect();
// diff must be remove all but last
let (diff, update_members) = calculate_diff(cur, vals.clone());
let (diff, add, remove) = calculate_diff(cur, vals.clone());
assert_eq!(
ValidatorDiff {
diffs: vals
Expand All @@ -1225,15 +1245,13 @@ mod test {
},
diff
);
assert!(add.is_empty());
assert_eq!(
update_members,
update_members_msg(
vals.iter()
.take(VALIDATORS - 1)
.map(|vi| vi.operator.as_ref())
.collect(),
vec![]
)
remove,
vals.iter()
.take(VALIDATORS - 1)
.map(|vi| vi.operator.as_ref())
.collect::<Vec<_>>()
);
}
}
2 changes: 2 additions & 0 deletions contracts/tgrade-valset/src/msg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ pub struct OperatorResponse {
pub pubkey: Pubkey,
pub metadata: ValidatorMetadata,
pub jailed_until: Option<JailingPeriod>,
pub active_validator: bool,
}

impl OperatorResponse {
Expand All @@ -330,6 +331,7 @@ impl OperatorResponse {
pubkey: info.pubkey.into(),
metadata: info.metadata,
jailed_until: jailed_until.into(),
active_validator: info.active_validator,
}
}
}
Expand Down
Loading