Abstracting API out of tests internals so it is clearly owned by App
#368
Conversation
hashedone
force-pushed
the
bank-keeper-api
branch
from
f6c4688 to
a31b724
Compare
hashedone
force-pushed
the
bank-keeper-api
branch
from
a31b724 to
e75b7d6
Compare
packages/multi-test/src/app.rs
Outdated
| .unwrap(); | ||
|
|
||
| // shows up in cache | ||
| let cached_rcpt = query_router(&app.router, &cache, &rcpt); | ||
| let cached_rcpt = query_router(&app.router, app.api.as_ref(), &cache, &rcpt); |
There was a problem hiding this comment.
| let cached_rcpt = query_router(&app.router, app.api.as_ref(), &cache, &rcpt); | |
| let cached_rcpt = query_router(&app.router, &*app.api, &cache, &rcpt); |
here and elsewhere?
There was a problem hiding this comment.
Honestly I personally find as_ref() more explicit, and what is more important - wider used also in this project. That is why I would keep it here for consistency. I don't have strong preference, and also I see you safe couple signs, and if you really want I would change it.
There was a problem hiding this comment.
It's OK. It just seems nice to me to have all parameters referenced in more or less the same way (with &). But it's the same.
There was a problem hiding this comment.
Then I will change it, I somehow agree with you. Today I was working on other think at afternoon, but I will do it as first thing at the morning an merge this (as you approved I assume there are no other issues).
There was a problem hiding this comment.
Either way. I'll defer to both your judgement for handling boxes (feel free to update other usages if desired... maybe as a separate PR)
| let bank_storage = prefixed_read(storage, NAMESPACE_BANK); | ||
| match request { | ||
| BankQuery::AllBalances { address } => { | ||
| // TODO: shall we pass in Api to make this safer? | ||
| let amount = self.get_balance(&bank_storage, &Addr::unchecked(address))?; | ||
| let address = api.addr_validate(&address).map_err(|err| err.to_string())?; |
| @@ -189,29 +197,31 @@ where | |||
| /// QuerierWrapper to interact with | |||
| pub fn query( | |||
| &self, | |||
| api: &dyn Api, | |||
There was a problem hiding this comment.
Great solution to this problem... pass it in!
| @@ -62,14 +63,15 @@ where | |||
| C: Clone + fmt::Debug + PartialEq + JsonSchema + 'static, | |||
| { | |||
| pub fn new( | |||
| api: Box<dyn Api>, | |||
There was a problem hiding this comment.
Nice to remove Box from the public API. Better this way.
(And on reflection from you, I think maybe generics for eg. Bank implementation makes sense.. but that is a different refactor).
There was a problem hiding this comment.
I would actually prefer to be generic over all those things (like Api and Storage), as internal optimizer tells me there is additional overhead of v-call here, but... come on, those are tests. This makes usage easier, as additional type hints are never an issue. I would not do that unless there is an actual need (I found sometimes dyn traits doesn't play well with typesystem, but I don't think here is a case).
There was a problem hiding this comment.
If you look at Deps and DepsMut we use dyn for Storage, Api and Querier. That change was made in 0.10 or 0.11. This made writing contracts much, much easier, especially for Rust newcomers.
I am very happy with that and would avoid re-introducing generics there. But for all other types, I am open. I just got in the habit after seeing how much that simplified the code.
packages/multi-test/src/app.rs
Outdated
| .unwrap(); | ||
|
|
||
| // shows up in cache | ||
| let cached_rcpt = query_router(&app.router, &cache, &rcpt); | ||
| let cached_rcpt = query_router(&app.router, app.api.as_ref(), &cache, &rcpt); |
There was a problem hiding this comment.
Either way. I'll defer to both your judgement for handling boxes (feel free to update other usages if desired... maybe as a separate PR)
closes #353
The reason behind whole change is that
Router(orWasmKeeper) is no longer the only user ofApi.The easy way is to keep
ApiinRc, and besides my worries about that, I tried this solution. UnfortunatelyBankKeeperwhich is eager to useApiis passed toAppafter it is configured, so it would require exposing information that things areRcinternally to the world, which I don't like, as it allows creator ofAppto keep his copy ofApp, and I don't like idea thatAppis nevermore owner ofApi. I would probably go this way if only I would be able to keepApp::newjust takingApiwith ownership - I even found a way, but it overcomplicates internals.Another shot is instead of keeping
Apiinternal forApp, just take it there as a borrow, and deal this borrow to another users. Unfortunately it doesn't have too much upsides - app still is nevermore clean owner ofApi, and lifetimes bounds everywhere makes thinks slightly less clean, which is not needed to happen in tests.Also it is possible to just clone
Apiaround, as it is pretty much trivial, but it would make it less flexible to extend it in future.I took a way, where
Apiis just passed to relevant calls, just likeStorageand other utilities. This isolates everything nicely, doesn't affect usage, and is very consistent with other interfaces.