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

Bound querier conversion #127

Merged
merged 5 commits into from
May 24, 2023
Merged

Bound querier conversion #127

merged 5 commits into from
May 24, 2023

Conversation

jawoznia
Copy link
Collaborator

PR3 of #28

@jawoznia jawoznia requested a review from hashedone May 24, 2023 13:27
@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #127 (1a8a4c1) into main (46cc1c2) will increase coverage by 0.05%.
The diff coverage is 85.18%.

@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
+ Coverage   79.96%   80.02%   +0.05%     
==========================================
  Files          32       33       +1     
  Lines        1692     1717      +25     
==========================================
+ Hits         1353     1374      +21     
- Misses        339      343       +4     
Impacted Files Coverage Δ
sylvia-derive/src/lib.rs 75.00% <ø> (ø)
sylvia-derive/src/message.rs 75.80% <ø> (ø)
sylvia-derive/src/remote.rs 81.25% <ø> (ø)
sylvia-derive/src/interfaces.rs 81.25% <81.25%> (ø)
sylvia/tests/querier.rs 93.10% <90.00%> (-2.14%) ⬇️
sylvia-derive/src/input.rs 86.66% <100.00%> (+0.22%) ⬆️

📣 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.

LGTM, except their constructors are not exactly proper (consistency) - just fix them and then merge.

Comment on lines 698 to 700
pub fn borrowed(&self, querier: &'a #sylvia ::cw_std::QuerierWrapper<'a, C>) -> Self {
Self::new(self.contract, querier)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, this one is on me as I messed up signature, but we want this to be:

pub fn new(contract: &'a #sylvia ::cw_std::Addr, querier: &'a #sylvia ::cw_std::QuerierWrapper<'a, C>) -> Self {
    Self {contract, querier}
}

So basically, your new should be called borrowed.

Comment on lines 694 to 696
pub fn new(contract: &'a #sylvia ::cw_std::Addr, querier: &'a #sylvia ::cw_std::QuerierWrapper<'a, C>) -> Self {
Self {contract, querier}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we ever need this function, but if so, then the signature is bad - Addr for this should be taken by owned value.

The reason this is probably needed is that if we store the address in the contract, then we probably store it as Remote, and we want to use the .querier() function to create a BoundQuerier. We use a borrowed(...) function only to make a shorthand to Remote::borrowed(addr).querier(querier), but then I would prefer it to be called borrowed just like in Remote.

@jawoznia jawoznia merged commit caf64a0 into main May 24, 2023
6 checks passed
@jawoznia jawoznia deleted the bound_querier_conversion branch May 24, 2023 14:20
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.

None yet

2 participants