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

Refactor Admin cw-controller to better represent actual functionality #424

Closed
ueco-jb opened this issue Sep 15, 2021 · 2 comments
Closed
Labels
good first issue Good for newcomers
Milestone

Comments

@ueco-jb
Copy link
Collaborator

ueco-jb commented Sep 15, 2021

Follow-up of https://github.com/confio/tgrade-contracts/pull/124#discussion_r709188534

When I read admin's tests I completely missed point of tested operations without reading implementation.
https://github.com/CosmWasm/cw-plus/blob/main/packages/controllers/src/admin.rs#L102

let control = Admin::new("foo");
let admin = Some(Addr::unchecked("admin"));
control.set(deps.as_mut(), admin.clone()).unwrap();
let got = control.get(deps.as_ref()).unwrap();
assert_eq!(admin, got);

Later this might be the case as well in production code.

I see some room for improvements in renaming those methods to better illustrace fact that operations are performed on addreses.

@ueco-jb ueco-jb added the good first issue Good for newcomers label Sep 15, 2021
@hashedone
Copy link
Contributor

Honestly I don't see it. In the example you quoted the problem is the name of variable - why is this control instead of admin? Let try it that way:

let admin = Admin::new("foo");
admin.set(deps.as_mut(), Some(Addr::unchecked("admin")).unwrap();
let admin_addr = admin.get(deps.as_ref()).unwrap();
assert_eq!(Some(Addr::unchecked("admin")), admin_addr);

The most strrange is execute_update_admin, but I don't see better name for it. However if you have idea for better names - feel free to propose them :)

@ethanfrey ethanfrey added this to the v0.10.0 milestone Sep 20, 2021
@ueco-jb
Copy link
Collaborator Author

ueco-jb commented Sep 20, 2021

However if you have idea for better names - feel free to propose them :)

I found it confusing at start. For me there's no clear connection that Admin is an Option<Addr>, which found it's epitome in test in which Admin was named control on top of that.

However if you assossiate Admin with an address, then methods like admin.get or set starts making sense...

I was considering something as simple as renaming methods to set_addr, but it doesn't make better sense since Addr is an optional so it's actually setting an admin, given that admin is this Item<'a, Option<Addr>>...

I will close this issue, because in order to satisfy my accusations I'd have to rewrite whole controller, which doesn't make sense as well since it's working well and everyone (eventually) will understand it.

@ueco-jb ueco-jb closed this as completed Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants