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

Conversation

orkunkl
Copy link
Contributor

@orkunkl orkunkl commented Sep 7, 2020

Implements permission functionality for messages:

  • Delegate
  • Undelegate
  • Redelegate
  • Withdraw

@orkunkl orkunkl requested a review from ethanfrey September 7, 2020 07:37
@CLAassistant
Copy link

CLAassistant commented Sep 7, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Thanks for your first contract PR @orkunkl

In general, you got all the pieces connected and it looks generally correct. I added a number of comments to improve code quality. Can you go through them and also take another pass on naming design and I will then re-review this

contracts/cw1-subkeys/README.md Show resolved Hide resolved
}

pub struct AllowanceResponse {
Copy link
Member

Choose a reason for hiding this comment

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

You removed the AllowanceResponse? I think you need both.

Oh, this is README....

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 wanted to make helpers.ts and msg.rs naming identical

@@ -135,9 +135,17 @@ const useOptions = (options: Options): Network => {

type Expiration = { at_height: { height: number } } | { at_time: { time: number } } | { never: {}}

interface Permissions {
Copy link
Member

Choose a reason for hiding this comment

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

Question:

If we add permissions for eg. governance votes, or calling wasm contracts, would you keep them here? Or use a separate type?

To see if we want one large "Permissions" or "StakingPermissions", "VotingPermissions", etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separate permissions design brings a lot of extra code(bucket) to both smart contract and helpers.ts. This struct will not contain hundreds of booleans(max 15-20?). I guess using single permissions struct is the optimal way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If rust and buckets provides a way to define permissions generically, then multiple permissions that extend Permissions trait is better.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, we keep one Permissions and extend it for other packages.
Please add a comment on the type explaining that choice (actually only in helpers.rs, not here)

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 assume you mean state.rs by helpers.rs? or helpers.ts? I am confused 😕

Copy link
Member

Choose a reason for hiding this comment

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

I meant state.rs
I should re-review again

contracts/cw1-subkeys/src/state.rs Show resolved Hide resolved
packages/cw0/src/expiration.rs Show resolved Hide resolved
contracts/cw1-subkeys/src/contract.rs Outdated Show resolved Hide resolved
contracts/cw1-subkeys/src/contract.rs Outdated Show resolved Hide resolved
contracts/cw1-subkeys/src/contract.rs Outdated Show resolved Hide resolved
contracts/cw1-subkeys/src/contract.rs Outdated Show resolved Hide resolved
contracts/cw1-subkeys/src/contract.rs Outdated Show resolved Hide resolved
@orkunkl orkunkl requested a review from ethanfrey September 7, 2020 15:57
@ethanfrey
Copy link
Member

Looks good. One comment requested, and please cargo fmt so the linter is happy. Then I will do the final review to merge

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Much nicer.

If you could cover a few more cases in the test code, I can merge this

@@ -150,8 +174,7 @@ interface InitMsg {
readonly mutable: boolean,
}

// TODO: define more of these
type CosmosMsg = SendMsg;
type CosmosMsg = SendMsg | DelegateMsg | UndelegateMsg | RedelegateMsg | WithdrawMsg
Copy link
Member

Choose a reason for hiding this comment

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

👍

for msg in &msgs {
match msg {
CosmosMsg::Staking(staking_msg) => {
let permissions = permissions(&mut deps.storage);
Copy link
Member

Choose a reason for hiding this comment

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

This is fine and explicit. I generally make this more compact.

let perm = permissions(&mut deps.storage).load(owner_raw.as_slice())?;
check_staking_permissions(staking_msg, perm)?;

But it is fine as is. (and with custom error message)

staking_msg: &StakingMsg,
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

contracts/cw1-subkeys/src/contract.rs Outdated Show resolved Hide resolved
contracts/cw1-subkeys/src/contract.rs Outdated Show resolved Hide resolved
QueryMsg::CanSend { sender, msg } => to_binary(&query_can_send(deps, sender, msg)?),
QueryMsg::AllAllowances { start_after, limit } => {
to_binary(&query_all_allowances(deps, start_after, limit)?)
}
QueryMsg::AllPermissions { start_after, limit } => {
Copy link
Member

Choose a reason for hiding this comment

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

Nice addition

}
}
CosmosMsg::Staking(staking_msg) => {
permissions_read(&deps.storage)
Copy link
Member

Choose a reason for hiding this comment

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

Interesting chain.

I think the ? and match approach for allowances is more legible. But nice that you learned a bit more about map and map_or and handling Options and Results. Key skills for working with rust

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just some old scala and functional programming skills λ

Copy link
Contributor Author

@orkunkl orkunkl Sep 9, 2020

Choose a reason for hiding this comment

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

Applied the ? and match case.
But can_send method looks weird to me. The return value is StdResult, but in tests, it is expected to be always boolean. Which throws the error information to the thrash 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Error should only throw if we have trouble parsing from disk, and we cannot prove that will never happen.

In all normal cases (non-corrupt db), we should get true or false. You would have to write invalid json to the key to trigger an error

// 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)

}

// spender2 cannot execute (no permission)
for msg in &msgs {
Copy link
Member

Choose a reason for hiding this comment

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

I would add a few more tests where one sender has eg. 2 permissions set, 2 unset and ensure that those two pass, and the other 2 fail.

Just a double check that there is no typo in which permission is checked for which message. And ensures future changes don't break this (what if we swap delegate and redelegate permission checks? these tests would still pass)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the test. Developer errors must be more distinctive and functional than just a str :( CosmWasm/cosmwasm#518

@orkunkl orkunkl requested a review from ethanfrey September 9, 2020 12:33
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good.

Two minor nits on code style. You can ideally change them (or ignore them if you disagree strongly) and then ready for merge.

})
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.

@@ -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.

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


// 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.

@orkunkl
Copy link
Contributor Author

orkunkl commented Sep 9, 2020

2 points left:

  1. StdErr which is taken care of. We can keep this PR on hold until it is there. It's a minor improve so good to merge IMO.
  2. This is important. In helpers.ts, wasm source URL is pointed to v0.1.1 tag. After merge, cli helper will be broken. It is a good practice to use tags in helpers.ts files but to make it work we need to release new version after every contract change.... I don't see a problem with releasing minor versions. @ethanfrey ideas?

@ethanfrey
Copy link
Member

helpers.ts is a bit of an issue. current flow is this:

  1. Create 0.2.1 tag of rust code
  2. Package is auto-built (20 minutes later)
  3. Update helpers.ts to point to new code binary

Meaning 0.2.1 helpers.ts points to 0.2.0 (or 0.1.1) code.

I am happy to make a new tag shortly and then we have the updated helpers, but this chicken-and-egg problem is what stops us from using tagged helpers.ts, which is the real solution. Any ideas for work-arounds?

@ethanfrey
Copy link
Member

I am happy to merge this when you give the 👍

And we can cut a release of cosmwasm-plus contract today

@orkunkl
Copy link
Contributor Author

orkunkl commented Sep 10, 2020

Houston, the shuttle is ready to launch 🚀 👍

@ethanfrey ethanfrey merged commit 77cf27e into master Sep 10, 2020
@ethanfrey ethanfrey deleted the cw1-subkeys-permissions branch September 10, 2020 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants