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.

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

Draft
wants to merge 59 commits into
base: dev
Choose a base branch
from
Draft

Drop locator #81

wants to merge 59 commits into from

Conversation

faust403
Copy link
Member

@faust403 faust403 commented May 7, 2024

No description provided.

@faust403 faust403 requested a review from albertandrejev May 7, 2024 10:35
Cargo.toml Outdated Show resolved Hide resolved
pub details: ChainDetails,
}

#[cw_serde]
Copy link
Collaborator

Choose a reason for hiding this comment

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

add
#[derive(QueryResponses)]

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • cw_ownable_query

Copy link
Collaborator

@ratik ratik left a 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();
Copy link
Collaborator

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

contracts/locator/src/contract.rs Outdated Show resolved Hide resolved
contracts/locator/src/contract.rs Outdated Show resolved Hide resolved
contracts/locator/src/contract.rs Outdated Show resolved Hide resolved
contracts/locator/src/contract.rs Outdated Show resolved Hide resolved
contracts/locator/src/contract.rs Outdated Show resolved Hide resolved
contracts/locator/src/error.rs Outdated Show resolved Hide resolved
contracts/locator/src/error.rs Outdated Show resolved Hide resolved
contracts/locator/src/contract.rs Outdated Show resolved Hide resolved
contracts/locator/src/msg.rs Outdated Show resolved Hide resolved
@faust403 faust403 requested a review from albertandrejev May 21, 2024 20:09
albertandrejev
albertandrejev previously approved these changes May 21, 2024
@faust403 faust403 requested a review from albertandrejev May 22, 2024 19:05
});
it('Query factory instances', async () => {
const { locator } = context.contracts;
const res = [...(await locator.queryFactoryInstances())];
Copy link
Collaborator

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);
Copy link
Collaborator

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?

Copy link
Member Author

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

Copy link
Collaborator

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

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"),
Copy link
Collaborator

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

Copy link
Collaborator

@ratik ratik left a 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

@faust403 faust403 requested a review from ratik May 29, 2024 11:02
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) {
Copy link
Collaborator

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

@faust403 faust403 requested a review from ratik June 3, 2024 12:44
ratik
ratik previously approved these changes Jun 3, 2024
@oldremez oldremez marked this pull request as draft June 27, 2024 09:45
@albertandrejev albertandrejev changed the base branch from feat/audit-fixes to dev August 13, 2024 09:19
@faust403 faust403 dismissed stale reviews from ratik and albertandrejev via 80a875b October 25, 2024 07:52
@faust403 faust403 mentioned this pull request Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants