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

Extend ContractData in multi-test #360

Merged
merged 5 commits into from
Jul 29, 2021
Merged

Extend ContractData in multi-test #360

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c4c0f05
Added `created` field for `ContractData` in multi-test
hashedone Jul 28, 2021
d5a2a3a
Doc comments added
hashedone Jul 28, 2021
50b3cab
Make `register_contract` test use `transactional` utility
hashedone Jul 28, 2021
5e90bd2
Linting correction
hashedone Jul 28, 2021
bb62fce
Postt review corrections
hashedone Jul 29, 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
197 changes: 143 additions & 54 deletions packages/multi-test/src/wasm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,20 @@ const CONTRACTS: Map<&Addr, ContractData> = Map::new("contracts");

pub const NAMESPACE_WASM: &[u8] = b"wasm";

/// Contract Data is just a code_id that can be used to lookup the actual code from the Router
/// We can add other info here in the future, like admin
/// Contract Data includes information about contract, equivalent of `ContractInfo` in wasmd
/// interface.
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
struct ContractData {
/// Identifier of stored contract code
pub code_id: usize,
}

impl ContractData {
fn new(code_id: usize) -> Self {
ContractData { code_id }
}
/// Address of node who initially instantiated the contract
Copy link
Member

Choose a reason for hiding this comment

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

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)

pub creator: Addr,
/// Optional address of node who can execute migrations
Copy link
Member

Choose a reason for hiding this comment

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

Again, change node

pub admin: Option<Addr>,
/// Optional metadata passed while contract instantiation
Copy link
Member

Choose a reason for hiding this comment

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

It is not so optional. You could add a validation that this is a non-empty string (but that is another PR)

Copy link
Contributor Author

@hashedone hashedone Jul 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm.. bug in protobuf comment. I will fix.

pub label: String,
/// Blockchain height in the moment of instanciating the contract
Copy link
Member

Choose a reason for hiding this comment

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

s/instanciating/instantiating/

pub created: u64,
}

pub trait Wasm<C>
Expand Down Expand Up @@ -237,14 +240,20 @@ where
Ok((contract_addr, res))
}
WasmMsg::Instantiate {
admin: _,
admin,
code_id,
msg,
funds,
label: _,
label,
} => {
let contract_addr =
Addr::unchecked(self.register_contract(storage, code_id as usize)?);
let contract_addr = Addr::unchecked(self.register_contract(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe register_contract should just return Addr?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, It does. This Addr::unchecked is completely unnecessary (remnant of old times)

storage,
code_id as usize,
sender.clone(),
admin.map(Addr::unchecked),
label,
block.height,
)?);
// move the cash
self.send(
storage,
Expand Down Expand Up @@ -394,14 +403,24 @@ where
&self,
storage: &mut dyn Storage,
code_id: usize,
creator: Addr,
admin: impl Into<Option<Addr>>,
label: String,
created: u64,
) -> Result<Addr, String> {
let mut wasm_storage = prefixed(storage, NAMESPACE_WASM);

if !self.codes.contains_key(&code_id) {
return Err("Cannot init contract with unregistered code id".to_string());
}
let addr = self.next_address(&wasm_storage);
let info = ContractData::new(code_id);
let info = ContractData {
code_id,
creator,
admin: admin.into(),
label,
created,
};
CONTRACTS
.save(&mut wasm_storage, &addr, &info)
.map_err(|e| e.to_string())?;
Expand Down Expand Up @@ -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
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 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.

keeper
.register_contract(
cache,
code_id + 1,
Addr::unchecked("foobar"),
Addr::unchecked("admin"),
"label".to_owned(),
1000,
)
.unwrap_err();

// cannot register contract with unregistered codeId
keeper
.register_contract(&mut cache, code_id + 1)
.unwrap_err();
// we can register a new instance of this code
let contract_addr = keeper
.register_contract(
cache,
code_id,
Addr::unchecked("foobar"),
Addr::unchecked("admin"),
"label".to_owned(),
1000,
)
.unwrap();

// 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", &[]);
Copy link
Member

Choose a reason for hiding this comment

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

yeah, each of these calls could be it's own transaction. And only the register_contract returns the contract_address successfully

let err = keeper
.call_instantiate(
contract_addr.clone(),
cache,
&mock_router(),
&block,
info,
b"{}".to_vec(),
)
.unwrap_err();
// StdError from contract_error auto-converted to string
assert_eq!(err, "Generic error: Init failed");

// now, we call this contract and see the error message from the contract
let info = mock_info("foobar", &[]);
let err = keeper
.call_instantiate(
contract_addr,
&mut cache,
&mock_router(),
&block,
info,
b"{}".to_vec(),
)
.unwrap_err();
// StdError from contract_error auto-converted to string
assert_eq!(err, "Generic error: Init failed");
// and the error for calling an unregistered contract
let info = mock_info("foobar", &[]);
let err = keeper
.call_instantiate(
Addr::unchecked("unregistered"),
cache,
&mock_router(),
&block,
info,
b"{}".to_vec(),
)
.unwrap_err();
// Default error message from router when not found
assert_eq!(err, "cw_multi_test::wasm::ContractData not found");

// and the error for calling an unregistered contract
let info = mock_info("foobar", &[]);
let err = keeper
.call_instantiate(
Addr::unchecked("unregistered"),
&mut cache,
&mock_router(),
&block,
info,
b"{}".to_vec(),
)
.unwrap_err();
// Default error message from router when not found
assert_eq!(err, "cw_multi_test::wasm::ContractData not found");
Ok(contract_addr)
})
.unwrap();

// and flush
cache.prepare().commit(&mut wasm_storage);
// verify contract data are as expected
let contract_data = CONTRACTS
Copy link
Member

Choose a reason for hiding this comment

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

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

.load(
&prefixed_read(&wasm_storage, NAMESPACE_WASM),
&contract_addr,
)
.unwrap();
assert_eq!(
contract_data,
ContractData {
code_id,
creator: Addr::unchecked("foobar"),
admin: Some(Addr::unchecked("admin")),
label: "label".to_owned(),
created: 1000,
}
);
}

#[test]
Expand All @@ -700,7 +753,16 @@ mod test {
let mut wasm_storage = MockStorage::new();
let mut cache = StorageTransaction::new(&wasm_storage);

let contract_addr = keeper.register_contract(&mut cache, code_id).unwrap();
let contract_addr = keeper
.register_contract(
&mut cache,
code_id,
Addr::unchecked("foobar"),
None,
"".to_owned(),
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 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.

1000,
)
.unwrap();

let payout = coin(100, "TGD");

Expand Down Expand Up @@ -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
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 also be transactional. I missed this in my refactor.

.register_contract(
&mut cache,
code_id,
Addr::unchecked("foobar"),
None,
"".to_string(),
1000,
)
.unwrap();
let payout1 = coin(100, "TGD");
let info = mock_info("foobar", &[]);
let init_msg = to_vec(&PayoutInitMessage {
Expand All @@ -825,7 +896,16 @@ mod test {
assert_payout(&keeper, cache, &contract1, &payout1);

// create contract 2 and use it
let contract2 = keeper.register_contract(cache, code_id).unwrap();
let contract2 = keeper
.register_contract(
cache,
code_id,
Addr::unchecked("foobar"),
None,
"".to_owned(),
1000,
)
.unwrap();
let info = mock_info("foobar", &[]);
let init_msg = to_vec(&PayoutInitMessage {
payout: payout2.clone(),
Expand All @@ -849,7 +929,16 @@ mod test {
assert_payout(&keeper, cache2, &contract2, &payout2);

// create a contract on level 2
let contract3 = keeper.register_contract(cache2, code_id).unwrap();
let contract3 = keeper
.register_contract(
cache2,
code_id,
Addr::unchecked("foobar"),
None,
"".to_owned(),
1000,
)
.unwrap();
let info = mock_info("johnny", &[]);
let init_msg = to_vec(&PayoutInitMessage {
payout: payout3.clone(),
Expand Down