-
Notifications
You must be signed in to change notification settings - Fork 12
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
Drop locator #81
base: dev
Are you sure you want to change the base?
Drop locator #81
Conversation
contracts/drop-locator/src/msg.rs
Outdated
pub details: ChainDetails, | ||
} | ||
|
||
#[cw_serde] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add
#[derive(QueryResponses)]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- cw_ownable_query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also in general this contract must get info from factory. So when you add a chain you add a factory contract address but not whole details of the deployment
add_chain.name.clone(), | ||
&add_chain.details.clone(), | ||
) | ||
.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make simple for loop to avoid unwraps
}); | ||
it('Query factory instances', async () => { | ||
const { locator } = context.contracts; | ||
const res = [...(await locator.queryFactoryInstances())]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
expect(res).toHaveLength(2); | ||
for (const instance of res) { | ||
expect(instance.addr).toHaveLength(66); | ||
expect(instance.contracts.token_contract).toHaveLength(66); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we compare with contracts addresses returned from corresponding factory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? It's a result of query from factory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean not just check contract address length but compare the result returned from the locator to the result returned from the corresponding factory
contracts/locator/src/tests.rs
Outdated
withdrawal_voucher_contract: String::from("withdrawal_voucher_contract2"), | ||
withdrawal_manager_contract: String::from("withdrawal_manager_contract2"), | ||
strategy_contract: String::from("strategy_contract2"), | ||
validators_set_contract: String::from("validators_set_contract2"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use "validators_set_contract2".to_string() next time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. but minor test issue
let matches: number = 0; | ||
for (const factory of context.contracts.factories) { | ||
const factoryState = await factory.queryState(); | ||
for (let i = 0; i < res.length; i += 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a blocker. for(const instance of res) {
No description provided.