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

Abstracting API out of tests internals so it is clearly owned by App #368

Merged
merged 2 commits into from
Aug 3, 2021
Merged

Abstracting API out of tests internals so it is clearly owned by App #368

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e75b7d6
Abstracting API out of tests internals so it is clearly owned by `App`
hashedone Aug 2, 2021
e512320
Unified box dereferencing
hashedone Aug 3, 2021
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
16 changes: 8 additions & 8 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions contracts/cw20-escrow/src/integration_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ use crate::msg::{CreateMsg, DetailsResponse, ExecuteMsg, InstantiateMsg, QueryMs

fn mock_app() -> App {
let env = mock_env();
let api = Box::new(MockApi::default());
let api = MockApi::default();
let bank = BankKeeper::new();

App::new(api, env.block, bank, Box::new(MockStorage::new()))
App::new(api, env.block, bank, MockStorage::new())
}

pub fn contract_escrow() -> Box<dyn Contract<Empty>> {
Expand Down
4 changes: 2 additions & 2 deletions contracts/cw3-fixed-multisig/src/integration_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ use cw_multi_test::{App, BankKeeper, Contract, ContractWrapper, Executor};

fn mock_app() -> App {
let env = mock_env();
let api = Box::new(MockApi::default());
let api = MockApi::default();
let bank = BankKeeper::new();

App::new(api, env.block, bank, Box::new(MockStorage::new()))
App::new(api, env.block, bank, MockStorage::new())
}

pub fn contract_cw3_fixed_multisig() -> Box<dyn Contract<Empty>> {
Expand Down
4 changes: 2 additions & 2 deletions contracts/cw3-flex-multisig/src/contract.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,10 +471,10 @@ mod tests {

fn mock_app() -> App {
let env = mock_env();
let api = Box::new(MockApi::default());
let api = MockApi::default();
let bank = BankKeeper::new();

App::new(api, env.block, bank, Box::new(MockStorage::new()))
App::new(api, env.block, bank, MockStorage::new())
}

// uploads code and returns address of group contract
Expand Down
1 change: 1 addition & 0 deletions packages/multi-test/clippy.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
too-many-arguments-threshold = 10
90 changes: 59 additions & 31 deletions packages/multi-test/src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ pub struct App<C = Empty>
where
C: Clone + fmt::Debug + PartialEq + JsonSchema + 'static,
{
pub router: Router<C>,
router: Router<C>,
api: Box<dyn Api>,
storage: Box<dyn Storage>,
block: BlockInfo,
}
Expand All @@ -41,7 +42,7 @@ where
{
fn raw_query(&self, bin_request: &[u8]) -> QuerierResult {
self.router
.querier(self.storage.as_ref(), &self.block)
.querier(self.api.as_ref(), self.storage.as_ref(), &self.block)
.raw_query(bin_request)
}
}
Expand All @@ -62,14 +63,15 @@ where
C: Clone + fmt::Debug + PartialEq + JsonSchema + 'static,
{
pub fn new(
api: Box<dyn Api>,
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

api: impl Api + 'static,
block: BlockInfo,
bank: impl Bank + 'static,
storage: Box<dyn Storage>,
storage: impl Storage + 'static,
) -> Self {
App {
router: Router::new(api, bank),
storage,
router: Router::new(bank),
api: Box::new(api),
storage: Box::new(storage),
block,
}
}
Expand Down Expand Up @@ -106,10 +108,16 @@ where
// meaning, wrap current state, all writes go to a cache, only when execute
// returns a success do we flush it (otherwise drop it)

let (block, router) = (&self.block, &self.router);
transactional(self.storage.as_mut(), |write_cache, _| {
let Self {
block,
router,
api,
storage,
} = self;

transactional(storage.as_mut(), |write_cache, _| {
msgs.into_iter()
.map(|msg| router.execute(write_cache, block, sender.clone(), msg))
.map(|msg| router.execute(api.as_ref(), write_cache, block, sender.clone(), msg))
.collect()
})
}
Expand Down Expand Up @@ -144,6 +152,7 @@ where
) -> Result<AppResponse, String> {
let msg = to_vec(msg).map_err(|e| e.to_string())?;
self.router.wasm.sudo(
self.api.as_ref(),
contract_addr.into(),
self.storage.as_mut(),
&self.router,
Expand All @@ -153,10 +162,7 @@ where
}
}

pub struct Router<C>
where
C: Clone + fmt::Debug + PartialEq + JsonSchema + 'static,
{
pub struct Router<C> {
pub wasm: Box<dyn Wasm<C>>,
pub bank: Box<dyn Bank>,
}
Expand All @@ -165,20 +171,22 @@ impl<C> Router<C>
where
C: Clone + fmt::Debug + PartialEq + JsonSchema + 'static,
{
pub fn new(api: Box<dyn Api>, bank: impl Bank + 'static) -> Self {
pub(super) fn new(bank: impl Bank + 'static) -> Self {
Router {
wasm: Box::new(WasmKeeper::new(api)),
wasm: Box::new(WasmKeeper::new()),
bank: Box::new(bank),
}
}

pub fn querier<'a>(
&'a self,
api: &'a dyn Api,
storage: &'a dyn Storage,
block_info: &'a BlockInfo,
) -> RouterQuerier<'a, C> {
RouterQuerier {
router: self,
api,
storage,
block_info,
}
Expand All @@ -189,29 +197,31 @@ where
/// QuerierWrapper to interact with
pub fn query(
&self,
api: &dyn Api,
Copy link
Member

Choose a reason for hiding this comment

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

Great solution to this problem... pass it in!

storage: &dyn Storage,
block: &BlockInfo,
request: QueryRequest<Empty>,
) -> Result<Binary, String> {
match request {
QueryRequest::Wasm(req) => {
self.wasm
.query(storage, &self.querier(storage, block), block, req)
.query(api, storage, &self.querier(api, storage, block), block, req)
}
QueryRequest::Bank(req) => self.bank.query(storage, req),
QueryRequest::Bank(req) => self.bank.query(api, storage, req),
_ => unimplemented!(),
}
}

pub fn execute(
&self,
api: &dyn Api,
storage: &mut dyn Storage,
block: &BlockInfo,
sender: Addr,
msg: CosmosMsg<C>,
) -> Result<AppResponse, String> {
match msg {
CosmosMsg::Wasm(msg) => self.wasm.execute(storage, &self, block, sender, msg),
CosmosMsg::Wasm(msg) => self.wasm.execute(api, storage, &self, block, sender, msg),
CosmosMsg::Bank(msg) => self.bank.execute(storage, sender, msg),
_ => unimplemented!(),
}
Expand All @@ -223,16 +233,23 @@ where
C: Clone + fmt::Debug + PartialEq + JsonSchema + 'static,
{
router: &'a Router<C>,
api: &'a dyn Api,
storage: &'a dyn Storage,
block_info: &'a BlockInfo,
}
impl<'a, C> RouterQuerier<'a, C>
where
C: Clone + fmt::Debug + PartialEq + JsonSchema + 'static,
{
pub fn new(router: &'a Router<C>, storage: &'a dyn Storage, block_info: &'a BlockInfo) -> Self {
pub fn new(
router: &'a Router<C>,
api: &'a dyn Api,
storage: &'a dyn Storage,
block_info: &'a BlockInfo,
) -> Self {
Self {
router,
api,
storage,
block_info,
}
Expand All @@ -256,7 +273,7 @@ where
};
let contract_result: ContractResult<Binary> = self
.router
.query(self.storage, self.block_info, request)
.query(self.api, self.storage, self.block_info, request)
.into();
SystemResult::Ok(contract_result)
}
Expand All @@ -278,18 +295,18 @@ mod test {

fn mock_app() -> App {
let env = mock_env();
let api = Box::new(MockApi::default());
let api = MockApi::default();
let bank = BankKeeper::new();

App::new(api, env.block, bank, Box::new(MockStorage::new()))
App::new(api, env.block, bank, MockStorage::new())
}

fn custom_app() -> App<CustomMsg> {
let env = mock_env();
let api = Box::new(MockApi::default());
let api = MockApi::default();
let bank = BankKeeper::new();

App::new(api, env.block, bank, Box::new(MockStorage::new()))
App::new(api, env.block, bank, MockStorage::new())
}

fn get_balance<C>(app: &App<C>, addr: &Addr) -> Vec<Coin>
Expand Down Expand Up @@ -717,14 +734,19 @@ mod test {
// TODO: check error?
}

fn query_router<C>(router: &Router<C>, storage: &dyn Storage, rcpt: &Addr) -> Vec<Coin>
fn query_router<C>(
router: &Router<C>,
api: &dyn Api,
storage: &dyn Storage,
rcpt: &Addr,
) -> Vec<Coin>
where
C: Clone + fmt::Debug + PartialEq + JsonSchema,
{
let query = BankQuery::AllBalances {
address: rcpt.into(),
};
let res = router.bank.query(storage, query).unwrap();
let res = router.bank.query(api, storage, query).unwrap();
let val: AllBalanceResponse = from_slice(&res).unwrap();
val.amount
}
Expand Down Expand Up @@ -758,11 +780,17 @@ mod test {
amount: coins(25, "eth"),
};
app.router
.execute(&mut cache, &app.block, owner.clone(), msg.into())
.execute(
app.api.as_ref(),
&mut cache,
&app.block,
owner.clone(),
msg.into(),
)
.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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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?

Copy link
Contributor Author

@hashedone hashedone Aug 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

assert_eq!(coins(25, "eth"), cached_rcpt);
let router_rcpt = query_app(&app, &rcpt);
assert_eq!(router_rcpt, vec![]);
Expand All @@ -774,13 +802,13 @@ mod test {
amount: coins(12, "eth"),
};
app.router
.execute(cache2, &app.block, owner, msg.into())
.execute(app.api.as_ref(), cache2, &app.block, owner, msg.into())
.unwrap();

// shows up in 2nd cache
let cached_rcpt = query_router(&app.router, read, &rcpt);
let cached_rcpt = query_router(&app.router, app.api.as_ref(), read, &rcpt);
assert_eq!(coins(25, "eth"), cached_rcpt);
let cached2_rcpt = query_router(&app.router, cache2, &rcpt);
let cached2_rcpt = query_router(&app.router, app.api.as_ref(), cache2, &rcpt);
assert_eq!(coins(37, "eth"), cached2_rcpt);
Ok(())
})
Expand Down
Loading