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

Parametrized App, ExecProxy and MigrateProxy #150

Merged
merged 14 commits into from
Jun 7, 2023
Merged

Parametrized App, ExecProxy and MigrateProxy #150

merged 14 commits into from
Jun 7, 2023

Conversation

jawoznia
Copy link
Collaborator

@jawoznia jawoznia commented Jun 2, 2023

@jawoznia jawoznia requested a review from hashedone June 2, 2023 15:03
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Merging #150 (0d18592) into main (89121a2) will increase coverage by 1.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
+ Coverage   80.65%   81.79%   +1.13%     
==========================================
  Files          36       36              
  Lines        1820     1774      -46     
==========================================
- Hits         1468     1451      -17     
+ Misses        352      323      -29     
Impacted Files Coverage Δ
sylvia-derive/src/multitest.rs 74.74% <ø> (+5.32%) ⬆️
sylvia/tests/custom_msg.rs 25.00% <ø> (ø)
sylvia/src/multitest.rs 82.35% <100.00%> (+13.60%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

I know you wanted to simplify, but we cannot go with BasicApp - we have to go for full App supporting custom modules

sylvia/src/multitest.rs Outdated Show resolved Hide resolved
sylvia/src/multitest.rs Outdated Show resolved Hide resolved
sylvia/src/multitest.rs Outdated Show resolved Hide resolved
sylvia/src/multitest.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

It needs more work. And some tests. Writing tests for it you will see compilation errors and you will learn how to properly handle generics.

sylvia/src/multitest.rs Outdated Show resolved Hide resolved
sylvia/src/multitest.rs Outdated Show resolved Hide resolved
sylvia/src/multitest.rs Outdated Show resolved Hide resolved
sylvia/src/multitest.rs Outdated Show resolved Hide resolved
sylvia/src/multitest.rs Outdated Show resolved Hide resolved
sylvia/src/multitest.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

Ok, deref doesn't work; thank you for trying it out. Good job on proving me wrong.

The generic App looks far better now; it probably will be good to go. Minor comment about default.

Please get rid of the deref impl, and fall back to .app(), app_mut(), other comments are optional. If you decide one is to be done but is a bit to much work, please make an issue out of the comment for the future.

I give you approve as those are changes I believe you will solve properly.

let version: ContractVersion =
query_contract_info(&app.app().wrap(), contract.contract_addr.to_string()).unwrap();
let version: ContractVersion = query_contract_info(
&(*app).borrow_mut().wrap(),
Copy link
Collaborator

@hashedone hashedone Jun 7, 2023

Choose a reason for hiding this comment

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

Nah, Deref doesn't work here as we would like to - we don't want to expose RefCell. It was my bad; you were right here. .app() was better (and we need to keep it), but because queries are common let's implement the QuerierWrapper directly on our App, so we can provide the direct wrap() function on Querier (and let's rename it to querier as the wrap name without the context is confusing to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tracked under #154

contracts/cw1-subkeys/src/multitest/tests.rs Outdated Show resolved Hide resolved
sylvia/src/multitest.rs Outdated Show resolved Hide resolved
sylvia/src/multitest.rs Outdated Show resolved Hide resolved
sylvia/src/multitest.rs Outdated Show resolved Hide resolved
@jawoznia jawoznia merged commit 196cad0 into main Jun 7, 2023
@jawoznia jawoznia deleted the generic_app branch June 7, 2023 14:32
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.

2 participants