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.

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

cw1-subkeys: Implement permission functionality #75

Merged
merged 36 commits into from
Sep 10, 2020
Merged
Changes from 2 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
63bc3c5
cw1-subkeys: introduce permissions
orkunkl Sep 2, 2020
6efffd9
cw1-subkeys: implement handle_execute checks
orkunkl Sep 3, 2020
bc9e5eb
cw0: implement display for expiration
orkunkl Sep 4, 2020
ad2f705
Fix tests
orkunkl Sep 4, 2020
91220d2
Add PermissionsErr
orkunkl Sep 4, 2020
cb2d0a5
Finish tests
orkunkl Sep 4, 2020
c65f00c
Generate schema
orkunkl Sep 4, 2020
c11e3ec
Commenting
orkunkl Sep 4, 2020
bc14bdc
Rename Permissions to Permission
orkunkl Sep 4, 2020
71ae543
Rename Permission to Permissions
orkunkl Sep 4, 2020
ef8c108
Add permission functionality to helper.ts
orkunkl Sep 4, 2020
acaac36
Improve helpers.ts
orkunkl Sep 6, 2020
0a91d03
Fix helpers.ts and add new staking msg types
orkunkl Sep 6, 2020
e427a62
helpers.ts works
orkunkl Sep 6, 2020
40133d3
Fix linter errs
orkunkl Sep 7, 2020
aaa19d7
Rename SetupPermissions msg to SetStakingPermissions
orkunkl Sep 7, 2020
96e1840
Apply review recommendations
orkunkl Sep 7, 2020
93f966f
Implement query methods for permissions
orkunkl Sep 7, 2020
ee0257d
Fix query
orkunkl Sep 7, 2020
4b0285d
Refactor query
orkunkl Sep 7, 2020
67ea0ea
Simplify permission check test
orkunkl Sep 7, 2020
db6193a
Add query tests
orkunkl Sep 7, 2020
8e87951
Refactor PermissionErr
orkunkl Sep 7, 2020
2fc834c
Improve helpers.ts
orkunkl Sep 7, 2020
6afd69b
Fix can send
orkunkl Sep 7, 2020
b539e36
Fix helpers.ts permission function
orkunkl Sep 7, 2020
339d8e6
Change staking permissions function name
orkunkl Sep 7, 2020
ec9f3ef
Generate schema
orkunkl Sep 7, 2020
b353307
Add permission comment
orkunkl Sep 8, 2020
bca69e1
Run cargo fmt
orkunkl Sep 8, 2020
00cab66
Fix linter errors
orkunkl Sep 8, 2020
78fbd27
Fix linter errors
orkunkl Sep 8, 2020
eb8d1f7
Apply review recommendations
orkunkl Sep 9, 2020
0ca0db1
Run cargo fmt
orkunkl Sep 9, 2020
3b49a64
Apply review recommendations
orkunkl Sep 9, 2020
e88048b
Fix err
orkunkl Sep 9, 2020
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
184 changes: 150 additions & 34 deletions contracts/cw1-subkeys/src/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,35 +127,22 @@ pub fn check_staking_permissions(
permissions: Permissions,
) -> Result<bool, PermissionErr> {
match staking_msg {
Copy link
Member

Choose a reason for hiding this comment

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

Much nicer

StakingMsg::Delegate {
validator: _,
amount: _,
} => {
StakingMsg::Delegate { .. } => {
if !permissions.delegate {
return Err(PermissionErr::Delegate {});
}
}
StakingMsg::Undelegate {
validator: _,
amount: _,
} => {
StakingMsg::Undelegate { .. } => {
if !permissions.undelegate {
return Err(PermissionErr::Undelegate {});
}
}
StakingMsg::Redelegate {
src_validator: _,
dst_validator: _,
amount: _,
} => {
StakingMsg::Redelegate { .. } => {
if !permissions.redelegate {
return Err(PermissionErr::Redelegate {});
}
}
StakingMsg::Withdraw {
validator: _,
recipient: _,
} => {
StakingMsg::Withdraw { .. } => {
if !permissions.withdraw {
return Err(PermissionErr::Withdraw {});
}
Expand Down Expand Up @@ -274,7 +261,9 @@ where
return Err(StdError::unauthorized());
}
if spender_raw == owner_raw {
return Err(StdError::generic_err("Cannot set allowance to own account"));
return Err(StdError::generic_err(
"Cannot set permission to own account",
));
}
permissions(&mut deps.storage).save(spender_raw.as_slice(), &perm)?;

Expand Down Expand Up @@ -365,14 +354,14 @@ fn can_send<S: Storage, A: Api, Q: Querier>(
}
}
CosmosMsg::Staking(staking_msg) => {
permissions_read(&deps.storage)
.may_load(owner_raw.as_slice())
.map(|perm_opt| {
// if permission exists
perm_opt.map_or(false, |perm| {
check_staking_permissions(&staking_msg, perm).is_ok()
})
})
let perm_opt = permissions_read(&deps.storage).may_load(owner_raw.as_slice())?;
match perm_opt {
Some(permission) => match check_staking_permissions(&staking_msg, permission) {
Copy link
Member

Choose a reason for hiding this comment

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

This could actually be:

Some(permission) => Ok(check_staking_permissions(&staking_msg, permission).is_ok())

Error only returned from may_load unable to parse.

Ok(..) => Ok(true),
Err(..) => Ok(false),
},
None => Ok(false),
}
}
_ => Ok(false),
}
Expand Down Expand Up @@ -690,11 +679,10 @@ mod tests {
// default is no permission
permissions: Default::default(),
};
// default is no permission
handle(&mut deps, env.clone(), setup_perm_msg2).unwrap();

let permissions = query_permissions(&deps, spender1.clone()).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Looks good. I would add one more test, where you set the allowance for someone with a permission and then ensure that both are set properly (setting allowance doesn't unset the permission)

assert_eq!(permissions, god_mode,);
assert_eq!(permissions, god_mode);

let permissions = query_permissions(&deps, spender2.clone()).unwrap();
assert_eq!(
Expand All @@ -718,6 +706,8 @@ mod tests {
withdraw: false
},
);

//
}

#[test]
Expand Down Expand Up @@ -1241,7 +1231,7 @@ mod tests {
}

#[test]
fn test_staking_permission_checks() {
fn staking_permission_checks() {
let mut deps = mock_dependencies(20, &[]);

let owner = HumanAddr::from("admin0001");
Expand All @@ -1253,7 +1243,7 @@ mod tests {
let spender2 = HumanAddr::from("spender0002");
let denom = "token1";
let amount = 10000;
let allow = coin(amount, denom);
let coin = coin(amount, denom);
Copy link
Member

Choose a reason for hiding this comment

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

This shadows the function coin for the rest of this test function. I would avoid that

(I only shadow one variable with another when transforming it, eg. let msg = msg.to_string();)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see 👍 Renamed it to coin1. allow was not a good fit there.


let god_mode = Permissions {
delegate: true,
Expand Down Expand Up @@ -1286,18 +1276,18 @@ mod tests {

let msg_delegate = vec![StakingMsg::Delegate {
validator: HumanAddr::from("validator1"),
amount: allow.clone(),
amount: coin.clone(),
}
.into()];
let msg_redelegate = vec![StakingMsg::Redelegate {
src_validator: HumanAddr::from("validator1"),
dst_validator: HumanAddr::from("validator2"),
amount: allow.clone(),
amount: coin.clone(),
}
.into()];
let msg_undelegate = vec![StakingMsg::Undelegate {
validator: HumanAddr::from("validator1"),
amount: allow.clone(),
amount: coin.clone(),
}
.into()];
let msg_withdraw = vec![StakingMsg::Withdraw {
Expand All @@ -1306,7 +1296,12 @@ mod tests {
}
.into()];

let msgs = vec![msg_delegate, msg_redelegate, msg_undelegate, msg_withdraw];
let msgs = vec![
msg_delegate.clone(),
msg_redelegate.clone(),
msg_undelegate.clone(),
msg_withdraw.clone(),
];

// spender1 can execute
for msg in &msgs {
Expand All @@ -1321,7 +1316,128 @@ mod tests {
let res = handle(&mut deps, env, HandleMsg::Execute { msgs: msg.clone() });
assert!(res.is_err())
}

// test mixed permissions
let spender3 = HumanAddr::from("spender0003");
let setup_perm_msg3 = HandleMsg::SetPermissions {
spender: spender3.clone(),
permissions: Permissions {
delegate: false,
redelegate: true,
undelegate: true,
withdraw: false,
},
};
handle(&mut deps, env.clone(), setup_perm_msg3).unwrap();
let env = mock_env(&spender3, &[]);
let res = handle(
&mut deps,
env.clone(),
HandleMsg::Execute {
msgs: msg_delegate.clone(),
},
);
// FIXME need better error check here
Copy link
Member

Choose a reason for hiding this comment

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

You want typed errors... I know we will make this happen

assert!(res.is_err());
let res = handle(
&mut deps,
env.clone(),
HandleMsg::Execute {
msgs: msg_redelegate.clone(),
},
);
assert!(res.is_ok());
let res = handle(
&mut deps,
env.clone(),
HandleMsg::Execute {
msgs: msg_undelegate.clone(),
},
);
assert!(res.is_ok());
let res = handle(
&mut deps,
env.clone(),
HandleMsg::Execute {
msgs: msg_withdraw.clone(),
},
);
assert!(res.is_err())
}

// tests permissions and allowances are independent features and does not affect each other
#[test]
fn permissions_allowances_independent() {
Copy link
Member

Choose a reason for hiding this comment

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

Thank you.

let mut deps = mock_dependencies(20, &[]);

let owner = HumanAddr::from("admin0001");
let admins = vec![owner.clone()];

// spender1 has every permission to stake
let spender1 = HumanAddr::from("spender0001");
let spender2 = HumanAddr::from("spender0002");
let denom = "token1";
let amount = 10000;
let coin = coin(amount, denom);

let allow = Allowance {
balance: Balance(vec![coin.clone()]),
expires: Expiration::Never {},
};
let perm = Permissions {
delegate: true,
redelegate: false,
undelegate: false,
withdraw: true,
};

let env = mock_env(owner.clone(), &[]);
// Init a contract with admins
let init_msg = InitMsg {
admins: admins.clone(),
mutable: true,
};
init(&mut deps, env.clone(), init_msg).unwrap();

// setup permission and then allowance and check if changed
let setup_perm_msg = HandleMsg::SetPermissions {
spender: spender1.clone(),
permissions: perm,
};
handle(&mut deps, env.clone(), setup_perm_msg).unwrap();

let setup_allowance_msg = HandleMsg::IncreaseAllowance {
spender: spender1.clone(),
amount: coin.clone(),
expires: None,
};
handle(&mut deps, env.clone(), setup_allowance_msg).unwrap();

let res_perm = query_permissions(&deps, spender1.clone()).unwrap();
assert_eq!(perm, res_perm);
let res_allow = query_allowance(&deps, spender1.clone()).unwrap();
assert_eq!(allow, res_allow);

// setup allowance and then permission and check if changed
let setup_allowance_msg = HandleMsg::IncreaseAllowance {
spender: spender2.clone(),
amount: coin.clone(),
expires: None,
};
handle(&mut deps, env.clone(), setup_allowance_msg).unwrap();

let setup_perm_msg = HandleMsg::SetPermissions {
spender: spender2.clone(),
permissions: perm,
};
handle(&mut deps, env.clone(), setup_perm_msg).unwrap();

let res_perm = query_permissions(&deps, spender2.clone()).unwrap();
assert_eq!(perm, res_perm);
let res_allow = query_allowance(&deps, spender2.clone()).unwrap();
assert_eq!(allow, res_allow);
}

#[test]
fn can_send_query_works() {
let mut deps = mock_dependencies(20, &[]);
Expand Down