Extend ContractData in multi-test
#360
Conversation
hashedone
force-pushed
the
350-extend-contract-data
branch
from
b60b2b7
to
d5a2a3a
Compare
|
I agree. |
packages/multi-test/src/wasm.rs
Outdated
| fn new(code_id: usize) -> Self { | ||
| ContractData { code_id } | ||
| } | ||
| /// Address of node who initially instantiated the contract |
There was a problem hiding this comment.
s/node/account/
A node is an observer who creates blocks.
A user or account is an actor who authorises tx
(I would use user, but contracts can also instantiate other contracts and have accounts)
packages/multi-test/src/wasm.rs
Outdated
| } | ||
| /// Address of node who initially instantiated the contract | ||
| pub creator: Addr, | ||
| /// Optional address of node who can execute migrations |
packages/multi-test/src/wasm.rs
Outdated
| pub creator: Addr, | ||
| /// Optional address of node who can execute migrations | ||
| pub admin: Option<Addr>, | ||
| /// Optional metadata passed while contract instantiation |
There was a problem hiding this comment.
It is not so optional. You could add a validation that this is a non-empty string (but that is another PR)
There was a problem hiding this comment.
I actually based those descriptions on protobuf message related to this issue, where it is optional, but ok, i will remove the "Optional" from it.
There was a problem hiding this comment.
Hmmm.. bug in protobuf comment. I will fix.
packages/multi-test/src/wasm.rs
Outdated
| pub admin: Option<Addr>, | ||
| /// Optional metadata passed while contract instantiation | ||
| pub label: String, | ||
| /// Blockchain height in the moment of instanciating the contract |
There was a problem hiding this comment.
s/instanciating/instantiating/
packages/multi-test/src/wasm.rs
Outdated
| } => { | ||
| let contract_addr = | ||
| Addr::unchecked(self.register_contract(storage, code_id as usize)?); | ||
| let contract_addr = Addr::unchecked(self.register_contract( |
There was a problem hiding this comment.
Maybe register_contract should just return Addr?
There was a problem hiding this comment.
Oh, It does. This Addr::unchecked is completely unnecessary (remnant of old times)
| @@ -647,48 +666,82 @@ mod test { | |||
| let block = mock_env().block; | |||
| let code_id = keeper.store_code(contract_error()); | |||
|
|
|||
| let mut cache = StorageTransaction::new(&wasm_storage); | |||
| let contract_addr = transactional(&mut wasm_storage, |cache, _| { | |||
| // cannot register contract with unregistered codeId | |||
There was a problem hiding this comment.
This could actually be in another transactional block.
To ensure it didn't make any changes when it returns an error.
Since they are simpler, we can do more.
packages/multi-test/src/wasm.rs
Outdated
| // we can register a new instance of this code | ||
| let contract_addr = keeper.register_contract(&mut cache, code_id).unwrap(); | ||
| // now, we call this contract and see the error message from the contract | ||
| let info = mock_info("foobar", &[]); |
There was a problem hiding this comment.
yeah, each of these calls could be it's own transaction. And only the register_contract returns the contract_address successfully
packages/multi-test/src/wasm.rs
Outdated
| code_id, | ||
| Addr::unchecked("foobar"), | ||
| None, | ||
| "".to_owned(), |
There was a problem hiding this comment.
This is actually illegal in wasmd and will be illegal in the near future in multi-test. Let's avoid that except for tests that should fail.
packages/multi-test/src/wasm.rs
Outdated
| @@ -798,7 +860,16 @@ mod test { | |||
| let mut cache = StorageTransaction::new(&wasm_storage); | |||
|
|
|||
| // set contract 1 and commit (on router) | |||
| let contract1 = keeper.register_contract(&mut cache, code_id).unwrap(); | |||
| let contract1 = keeper | |||
There was a problem hiding this comment.
This could also be transactional. I missed this in my refactor.
packages/multi-test/src/wasm.rs
Outdated
| // and flush | ||
| cache.prepare().commit(&mut wasm_storage); | ||
| // verify contract data are as expected | ||
| let contract_data = CONTRACTS |
There was a problem hiding this comment.
We should not do this in test code. Actually WasmKeeper.load_contract() does this.
We should make it public and expose it in the Wasm interface (and exposed in App as well, so we can use it in higher-level tests).
It might also be nice to have a query that returns an Iterator of all ContractData (but we can add that later if needed). This is a query we have in wasmd, but no need to add it yet
|
I didn't refactor tests to use |
One thing I would like to improve later about this is to make unit test to actually using
Wasm<C>interface instead of internal private functions.There are two reasons for that:
Such tests would be more stable so easier to maintain - internal private functions and layouts might change as it is their nature, but every such change requires tests update. Public API (which
Wasm<C>is forWasmKeeper) should probably not change almost at all (possibly might be extended, but unless some serious breaking change is made, it should not affect existing code, including tests).Wasm<C>implementation ofWasmKeeperare not just recalls to internall private functions, they actually have some orchestration logic. Testing basing on private calls, completely excludes this functonality from being tested.