Sylvia upgrade #188

JakeHartnell · 2024-04-28T22:45:07Z

Attempts to upgrade to Sylvia v0.10, following roughly what's in this article: https://medium.com/confio/sylvia-1-0-0-45094b6ad8ba

The rational for v0.10 over 1.0 is that the Starship testing infra chains don't yet support CW 2.0 I believe.

Doesn't fully work yet... would appreciate some help.

Builds off of #187, but separate for easier review.

JakeHartnell · 2024-04-28T22:47:29Z

contracts/consumer/converter/src/contract.rs

+        // FIXME not sure how to get this to compile with latest sylvia
        // get the price value (usage is a bit clunky, need to use trait and cannot chain Remote::new() with .querier())
        // also see https://github.com/CosmWasm/sylvia/issues/181 to just store Remote in state
        use price_feed_api::Querier;


@hashedone not sure what's changed here, but this part is very broken. Struggling to find examples for this Querier / Remote stuff...

Since 0.9.0 Sylvia generates all the code in the sv module. We decided not to populate the scope with all the stuff it generates and rather keep it in this module.

Currently both interface and contract generates the code like this.

pub mod sv { // messages // qoc code pub mod mt { } }

So for your code you have to:

+ use price_feed_api::sv::Querier; - use price_feed_api::Querier;

You can read about the changes regarding 0.10.0 in the release article https://medium.com/confio/sylvia-1-0-0-45094b6ad8ba
And about the 0.9.0 in https://medium.com/confio/sylvia-0-9-0-78f50984a75f

I noticed that the https://github.com/CosmWasm/sylvia/blob/main/MIGRATING.md is not fully updated with the changes. I will update it today. Sorry for inconvenience.

I noticed that the https://github.com/CosmWasm/sylvia/blob/main/MIGRATING.md is not fully updated with the changes. I will update it today. Sorry for inconvenience.

This would be super helpful.

How do I derive the Remote? Read the issue here and not sure what to make of it: CosmWasm/sylvia#181

Both Remote and BoundQuerier were moved to the sylvia::types module as they were always generated in the same way.

Here is example usage of Remote/Querier. If you find them insufficient and/or have some suggestions what is missing we will appreciate the feedback.
https://github.com/CosmWasm/sylvia/blob/main/sylvia/tests/querier.rs

Those missing derives were implemented in CosmWasm/sylvia#194 and are still present.
https://docs.rs/sylvia/0.10.1/sylvia/types/struct.Remote.html

Which traits are missing for your use case?

Hi @jawoznia I'm coding again on mesh and jumping in this PR.

I made the above changes, but then need a concrete type for remote (when I really just want to pass in an interface). This fixes some of the changes you said:

use price_feed_api::sv::Querier; use sylvia::types::Remote; let remote = Remote::new(config.price_feed); let price = remote.querier(&deps.querier).price()?.native_per_foreign;

I get the following two errors:

error[E0282]: type annotations needed for `Remote<'_, Contract>` --> contracts/consumer/converter/src/contract.rs:280:13 | 280 | let remote = Remote::new(config.price_feed); | ^^^^^^ ------------------------------ type must be known at this point | help: consider giving `remote` an explicit type, where the type for type parameter `Contract` is specified | 280 | let remote: Remote<'_, Contract> = Remote::new(config.price_feed); | ++++++++++++++++++++++

Problem here is it wants a concrete contract type, and made an error when I tried to pass in Remote::<PriceFeedApi>. One of the main poitns of the APIs was to be able to call them without knowing the implementation. Is there an example how to use remote to call an API not a concrete contract?

error[E0283]: type annotations needed --> contracts/consumer/converter/src/contract.rs:281:51 | 281 | let price = remote.querier(&deps.querier).price()?.native_per_foreign; | ^^^^^ | = note: multiple `impl`s satisfying `BoundQuerier<'_, cosmwasm_std::Empty, _>: mesh_apis::price_feed_api::sv::Querier` found in the `mesh_apis` crate: - impl<'a, C, Contract> mesh_apis::price_feed_api::sv::Querier for BoundQuerier<'a, C, Contract> where C: cosmwasm_std::CustomQuery, Contract: PriceFeedApi; - impl<'a, C, Error> mesh_apis::price_feed_api::sv::Querier for BoundQuerier<'a, C, &dyn PriceFeedApi<Error = Error>> where C: cosmwasm_std::CustomQuery; help: try using a fully qualified path to specify the expected types | 281 | let price = <BoundQuerier<'_, cosmwasm_std::Empty, Contract> as mesh_apis::price_feed_api::sv::Querier>::price(&remote.querier(&deps.querier))?.native_per_foreign; | ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

Okay, it is a little uglier to pass the querier as an argument to price, but no big deal there. The bigger issue is do I really need to make that complex type BoundQuerier<'_, cosmwasm_std::Empty, Contract> as mesh_apis::price_feed_api::sv::Querier> in my code? Surely there is a shorter way.

(Looking at your example it does seem we need the verbose way, but there are some shortcuts there for types)

Okay, please look at 71dffdf and let me know if that was the proper way to deal with it. I feel I am just forcing the type system rather then understanding anything

JakeHartnell · 2024-04-28T22:50:38Z

contracts/consumer/virtual-staking/src/contract.rs

+     */
+    fn handle_epoch(
+        &self,
+        ctx: SudoCtx<VirtualStakeCustomQuery>,


These two sudo methods (handle_epoch and valset_update) require custom queries and messages. Any guidance or suggestions on how to handle this would be greatly appreciated. 🙏

Examples or commits would be 🍰 .

I checked the branch and found out that some Ctx types miss the <VirtualStakeCustomQuery> parameter and same for CustomMsg for Response.

From Rust POV Response<Empty> and Response<VirtualStakeCustomMsg> are different types and you have to make all the methods in the dispatch flow use the same types.

JakeHartnell · 2024-04-28T22:56:52Z

contracts/consumer/virtual-staking/src/multitest.rs

 struct SetupResponse<'a> {
-    price_feed:
-        mesh_simple_price_feed::contract::multitest_utils::SimplePriceFeedContractProxy<'a, MtApp>,
-    converter: mesh_converter::contract::multitest_utils::ConverterContractProxy<'a, MtApp>,
-    virtual_staking: contract::multitest_utils::VirtualStakingContractProxy<'a, MtApp>,
+    price_feed: mesh_simple_price_feed::contract::sv::mt::SimplePriceFeedContractProxy<'a, MtApp>,
+    converter: mesh_converter::contract::sv::mt::ConverterContractProxy<'a, MtApp>,
+    virtual_staking: contract::sv::mt::VirtualStakingContractProxy<'a, MtApp>,
 }

 fn setup<'a>(app: &'a App<MtApp>, args: SetupArgs<'a>) -> SetupResponse<'a> {


I'm having a hard time with these setup functions... and not just in this file. It's generic and trait hell.

Getting tones of errors like this:

error[E0107]: trait takes 9 generic arguments but 1 generic argument was supplied --> contracts/provider/native-staking-proxy/src/multitest.rs:59:16 | 59 | ) -> AnyResult<VaultContractProxy<'app, MtApp>> { | ^^^^^^^^^^^^^^^^^^ ----- supplied 1 generic argument | | | expected 9 generic arguments | note: trait defined here, with 9 generic parameters: `BankT`, `ApiT`, `StorageT`, `CustomT`, `WasmT`, `StakingT`, `DistrT`, `IbcT`, `GovT` --> /Users/meow/Dev/Mesh/mesh-security/contracts/provider/vault/src/contract.rs:69:6 | 66 | #[contract] | ----------- ... 69 | impl VaultContract<'_> { | ^^^^^^^^^^^^^ help: add missing generic arguments | 59 | ) -> AnyResult<VaultContractProxy<'app, MtApp, ApiT, StorageT, CustomT, WasmT, StakingT, DistrT, IbcT, GovT>> { | ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ error[E0782]: trait objects must include the `dyn` keyword --> contracts/provider/native-staking-proxy/src/multitest.rs:59:16 | 59 | ) -> AnyResult<VaultContractProxy<'app, MtApp>> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | help: add `dyn` keyword before this trait | 59 | ) -> AnyResult<dyn VaultContractProxy<'app, MtApp>> { | +++

Too tired to figure it out right now, but any help would be appreciated.

expected 9 generic arguments

🥲

Proxy trait generated by Sylvia is generic over cw_multi_test modules while it would be much more user friendly to be generic over cw_multi_test::App.
I see the missing use case in our tests. I will inspect possibility to make it generic over the cw_multi_test::App.

For now to make it work you can provide the types used in the cw_multi_test::App that I see you are trying to use here
https://github.com/CosmWasm/cw-multi-test/blob/main/src/app.rs#L51

Even better. You are returning here the trait. Change it to https://docs.rs/sylvia/0.10.1/sylvia/multitest/struct.Proxy.html.

You will be able to provide the MtApp here as first parameter and your contract as a second generic parameter.

jawoznia · 2024-04-29T11:05:51Z

contracts/consumer/converter/src/contract.rs

-#[error(ContractError)]
-#[messages(converter_api as ConverterApi)]
+#[sv::error(ContractError)]
+#[sv::messages(converter_api as ConverterApi)]


Since 0.10.0 below is possible since we can deduce the interface name by converting the path to camel case

Suggested change

#[sv::messages(converter_api as ConverterApi)]

#[sv::messages(converter_api)]

jawoznia · 2024-04-29T11:11:56Z

contracts/consumer/virtual-staking/src/contract.rs

@@ -69,7 +71,7 @@ impl VirtualStakingContract<'_> {
    }

    /// The caller of the instantiation will be the converter contract
-    #[msg(instantiate)]
+    #[sv::msg(instantiate)]
    pub fn instantiate(&self, ctx: InstantiateCtx) -> Result<Response, ContractError> {


Since the contract is now using some custom message and query you have to use them in the methods signatures. Otherwise you get this errors with conversion from DepsMut<'_, Empty> to DepsMut<'_, VirtualStakeCustomQuery>

Suggested change

pub fn instantiate(&self, ctx: InstantiateCtx) -> Result<Response, ContractError> {

pub fn instantiate(&self, ctx: InstantiateCtx<VirtualStakeCustomQuery>) -> Result<Response<VirtualStakeCustomMsg>, ContractError> {

jawoznia

@JakeHartnell, @ethanfrey

Please check 528c6e1 with the update of consumer/converter. Tests are building and passing.

ethanfrey

Thanks for the help @jawoznia

Left a few comments and one big question

ethanfrey · 2024-05-06T17:30:09Z

contracts/consumer/converter/src/multitest.rs

-        mesh_simple_price_feed::contract::multitest_utils::SimplePriceFeedContractProxy<'a, MtApp>,
-    converter: contract::multitest_utils::ConverterContractProxy<'a, MtApp>,
-    virtual_staking: virtual_staking_mock::multitest_utils::VirtualStakingMockProxy<'a, MtApp>,
+    price_feed: Proxy<'a, MtApp, SimplePriceFeedContract<'a>>,


Okay, this is a lot of non-intuitive changes, but happy to use this as an example to fix others.

This change is because we moved out of generating Proxy type per contract macro.

Instead we have a sylvia::multitest::Proxy type that is generic over MtApp on which the SimplePriceFeedContract is deployed. So the idea was to link via proxy some contract to the chain.

ethanfrey · 2024-05-06T17:30:49Z

contracts/consumer/converter/src/multitest.rs

-        mesh_simple_price_feed::contract::multitest_utils::CodeId::store_code(app);
-    let virtual_staking_code = virtual_staking_mock::multitest_utils::CodeId::store_code(app);
-    let converter_code = contract::multitest_utils::CodeId::store_code(app);
+    let price_feed_code = PriceFeedCodeId::store_code(app);


Usage actually looks simpler

ethanfrey · 2024-05-06T17:31:31Z

contracts/consumer/converter/src/contract.rs

+        use sylvia::types::Remote;
+        // NOTE: Jan, this feels hacky... I'm not sure if this is the right way to do it
+        // Also, I am sticking in a random error type here, not what I will get (which is unknown)
+        let remote = Remote::<&dyn price_feed_api::PriceFeedApi<Error=StdError>>::new(config.price_feed);


Is this line correct? I feel I am faking the Remote generic type so it compiles... is this the right way?

This is correct, as sylvia::types::Remote being representation of some external Contract is generic over it.

Unfortunately Rust requires us to state the associated types defined on the PriceFeedApi trait which is not ideal, but we went with it as this way we were able to simplify support for generics and overall make the API less clunky (not without sacrifices as we can see).
In current state and in this usage you are using the Remote to instantiate BoundQuerier and query on contract.
It's fine to set Error as StdError as underneath it calls query_wasm_smart which returns the StdResult so although it looks hacky it's all fine.

The problem however is with storing Remote as a field, which we didn't cover for the Remote generic over some type implementing the interface and we have to fix it.

ethanfrey · 2024-05-09T12:32:58Z

@jawoznia I managed to fix a lot with your example. All code and tests compile properly - expect vitrual-staking. Some tests are failing (on asserts not equaling), but I will investigate the diff to try to figure out why.

The biggest issue is virtual-staking that uses custom query and message type. I made a number of changes to get consistency everywhere and it compiles and seems proper, except multitest fails. The biggest issue is when we use the app with custom msg/query (needed for virtual staking) and use it to instantiate another contract (that has no custom msg/query), then it fails like this:

error[E0308]: mismatched types
  --> contracts/consumer/virtual-staking/src/multitest.rs:42:79
   |
42 |     let converter_code = mesh_converter::contract::sv::mt::CodeId::store_code(app);
   |                          ---------------------------------------------------- ^^^ expected `&App<App<_, _, _, _, WasmKeeper<Empty, Empty>, _, _, _, _>>`, found `&App<App<BankKeeper, MockApi, MemoryStorage, ..., ...>>`
   |                          |
   |                          arguments to this function are incorrect
   |
   = note: expected reference `&sylvia::multitest::App<cw_multi_test::App<_, _, _, _, WasmKeeper<cosmwasm_std::Empty, cosmwasm_std::Empty>, _, _, _, _>>`
              found reference `&'a sylvia::multitest::App<cw_multi_test::App<BankKeeper, MockApi, MemoryStorage, FailingModule<mesh_bindings::VirtualStakeCustomMsg, mesh_bindings::VirtualStakeCustomQuery, cosmwasm_std::Empty>, WasmKeeper<mesh_bindings::VirtualStakeCustomMsg, mesh_bindings::VirtualStakeCustomQuery>>>`

ethanfrey · 2024-05-10T09:26:51Z

contracts/provider/vault/src/multitest.rs

+        block.time = block.time.plus_seconds(61);
+        block.height += 13;
+    });
+    // This is deprecated as unneeded, but tests fail if it isn't here. What's up???


@jawoznia another one as possible bug in the cosmwasm frameworks...

This is marked deprecated, saying I should just call update_block. But if I comment these lines out the tests fail.
I even tried to update the block with some huge diff (610.000.000 seconds) and it doesn't trigger the process queue.

Can you investigate? I know it is cw-multi-test, but still...

If this is not yet an issue in cw-multi-test please consider making this an issue there. Darek should definitely check it.

contracts in mt

Workaround to enable communication between `Empty` and `Custom` contracts in mt

JakeHartnell

LGTM! Thanks @ethanfrey and @jawoznia for helping get this updated!

JakeHartnell requested review from ethanfrey, hashedone and jawoznia April 28, 2024 22:45

JakeHartnell commented Apr 28, 2024

View reviewed changes

jawoznia reviewed Apr 29, 2024

View reviewed changes

Base automatically changed from upgrade-deps to main May 6, 2024 09:08

Jake Hartnell and others added 3 commits May 6, 2024 11:08

Upgrade Sylvia to 0.10, fix as many things as possible

9f81b06

Attempt to fix up virtual staking, still doesn't compile

94171cc

Got consumer/converter to compile (but not tests)

71dffdf

ethanfrey force-pushed the sylvia-upgrade branch from 4ce0893 to 71dffdf Compare May 6, 2024 09:09

ethanfrey and others added 2 commits May 6, 2024 11:17

Remove codecov as we only get errors (seems it needs a paid API key?)

437fcc4

Use sylvia::types::Proxy in consumer converter tests

528c6e1

jawoznia reviewed May 6, 2024

View reviewed changes

ethanfrey reviewed May 6, 2024

View reviewed changes

ethanfrey added 6 commits May 9, 2024 12:04

Clippy also happy for converter contract

a34baf2

Virtual-staking compiles

12605d3

Lost in a message of custom msg/query types

1d3772e

External and native staking compile, some tests fail

632ee1b

Everything runs minus virtual-staking, some tests fail

9b2be17

Clean up obvious errors in virtual-staking

180aa1e

ethanfrey added 3 commits May 9, 2024 14:37

More virtual staking cleanup

3fa51d7

Restore original jailing logic

19b17c2

Native-staking* and vault tests pass

c4c34f6

ethanfrey reviewed May 10, 2024

View reviewed changes

jawoznia and others added 3 commits May 13, 2024 16:21

Workaround to enable communication between Empty and Custom

3db923e

contracts in mt

More generic, simple-price and converter pass tests

07fb1ca

virtual-staking tests work

1009590

ethanfrey and others added 4 commits May 13, 2024 16:29

Test pass, clippy happy :)

4938265

Wasm compilation proper again

3092f95

More clippy fixes

c5bbcc4

Merge pull request #189 from osmosis-labs/sylvia-upgrade-custom-empty

19e50b2

Workaround to enable communication between `Empty` and `Custom` contracts in mt

ethanfrey marked this pull request as ready for review May 13, 2024 16:04

JakeHartnell commented May 13, 2024

View reviewed changes

JakeHartnell merged commit 4e8224b into main May 13, 2024
2 checks passed

JakeHartnell deleted the sylvia-upgrade branch May 13, 2024 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sylvia upgrade #188

Sylvia upgrade #188

JakeHartnell commented Apr 28, 2024 •

edited

Loading

JakeHartnell Apr 28, 2024

jawoznia Apr 29, 2024 •

edited

Loading

JakeHartnell Apr 29, 2024

jawoznia Apr 29, 2024

jawoznia Apr 29, 2024

ethanfrey May 6, 2024 •

edited

Loading

ethanfrey May 6, 2024

JakeHartnell Apr 28, 2024

jawoznia Apr 29, 2024

JakeHartnell Apr 28, 2024

JakeHartnell Apr 28, 2024

jawoznia Apr 29, 2024 •

edited

Loading

jawoznia Apr 29, 2024

jawoznia Apr 29, 2024

jawoznia Apr 29, 2024

jawoznia Apr 29, 2024

jawoznia left a comment

ethanfrey left a comment

ethanfrey May 6, 2024

jawoznia May 6, 2024

ethanfrey May 6, 2024

ethanfrey May 6, 2024

jawoznia May 6, 2024

ethanfrey commented May 9, 2024

ethanfrey May 10, 2024

jawoznia May 20, 2024

JakeHartnell left a comment

	#[sv::messages(converter_api as ConverterApi)]
	#[sv::messages(converter_api)]

	pub fn instantiate(&self, ctx: InstantiateCtx) -> Result<Response, ContractError> {
	pub fn instantiate(&self, ctx: InstantiateCtx<VirtualStakeCustomQuery>) -> Result<Response<VirtualStakeCustomMsg>, ContractError> {

Sylvia upgrade #188

Sylvia upgrade #188

Conversation

JakeHartnell commented Apr 28, 2024 • edited Loading

Choose a reason for hiding this comment

jawoznia Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jawoznia Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jawoznia left a comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeHartnell left a comment

Choose a reason for hiding this comment

JakeHartnell commented Apr 28, 2024 •

edited

Loading

jawoznia Apr 29, 2024 •

edited

Loading

ethanfrey May 6, 2024 •

edited

Loading

jawoznia Apr 29, 2024 •

edited

Loading