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

Adding per-message conditions and executing custom warp account messages via jobs #76

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Rhaki
Copy link

@Rhaki Rhaki commented Nov 11, 2023

This PR mainly wants to implement two new features.

  1. specific conditions for individual messages within a job (see iusse - 72);
  2. possibility to run custom warp account messages (like IbcTransfer).

The proposed changes are as follows:

  1. The general conditions (the current ones so to speak) are now optional, and make the entire tx fail if not respected.

  2. The msgs field remains a String (for backwards compatibility), however there are substantial changes in the HydrateMsgs query of resolver:

    • The msg as String is first deserialized into Vec<serde_cw_wasm::Value>.
    • Unlike before that the content of each message had to be a stringified ComosMsg, now a check is made whether it is a stringified CosmosMsg (backwards compatibility) or a stringified json with the following rappresentation:
    pub struct Foo {
      pub condition: Option<String>,
      pub msg: String
    }

    If condition is provided, it deserliazed to Condition and and checks whether they are met. If not met, the message is skipped.
    The msg must be a stringified version of the following enum:

    #[cw_serde]
      pub enum WarpMsg {
          Generic(CosmosMsg),
          IbcTransfer(IbcTransferMsg),
      }
    • The hydration part of variables in message remained the same;
    • The return of the function is no longer a Vec<CosmoMsg> but a Vec<WarpMsg>, which allows to manage any type of message that Warp wants to manage other than as ComsosMsg directly (such as the IbcTransfer);
    • In case it is a job that still uses the CosmosMsg directly, this is wrapped at the end of hydration as WarpMsg::Generic(msg as CosmosMsg);
    • If after the hydration of the messages, there are no messages to send (all the specific conditions of the individual messages have failed), the controller returns an error reverting the tx (and therefore allowing the keepers during the simulation to skip the execution of this job);
    • A new ExecuteMsg has been added for the warp account which allows to execute a Vec<WarpMsg>, thus storing the value and returning it directly the CosmosMsg on WarpMsg::Generic, otherwise the IbcTransfer is created by using the functions already present.

With this setting is possible to add other custom messages for the warp account where the CosmosMsg is not enough.

@llllllluc
Copy link
Contributor

wooo new warp contributor! welcome @Rhaki ! can you rebase your PR on top of branch feature/latest_updates? that one has a lotttt of change from the master branch

@0x7183
Copy link

0x7183 commented Nov 11, 2023

Hey @llllllluc this is our version of that branch, the version in feature/latest_updates is not compatible with old applications working on Warp.

We are building a Grid bot on top of warp that is being audited right now, we highly think that the backward compatibility is a must for a protocol like Warp that aims to give builders an easier way to build job applications on chain.

Please review it and take in consideration the idea of maintaining the backward compatibility!

Thank you

@llllllluc
Copy link
Contributor

feature/latest_updates is not compatible with old applications working on Warp

I would suggest we stop building new feature on top of master branch as feature/latest_updates is the next release version and it's already in testnet, indeed it's making some breaking changes. but for apps building on top of warp, it shouldn't have too much impact, if it does then it's probably worth to adapt it cause one major change in feature/latest_updates is introducing job account (#74 ), and it solves a lot of issues like how to make sure different job won't spend each other's fund by accident which is a huge pain in the old version (master branch, also mainnet version).

That is my opinion, @simke9445 please chime in

@llllllluc
Copy link
Contributor

llllllluc commented Nov 11, 2023

Hey @llllllluc this is our version of that branch, the version in feature/latest_updates is not compatible with old applications working on Warp.

We are building a Grid bot on top of warp that is being audited right now, we highly think that the backward compatibility is a must for a protocol like Warp that aims to give builders an easier way to build job applications on chain.

Please review it and take in consideration the idea of maintaining the backward compatibility!

Thank you

I can also modify this branch and rebase it on top of feature/latest_updates probably sometime next week or next next week, if you guys are not in a rush.

@0x7183
Copy link

0x7183 commented Nov 12, 2023

While I usually agree adding braking changes on a product that should be the base for others, it's not the best route.

Right now we are building a grid bot on top of Warp that is getting audited.

Those changes broke our contracts on Testnet, if we'll need to develop them again we'll probably avoid Warp because the risk that braking changes will be implemented in future are too high and we surely don't want to keep changing and migrating contracts to stay up to date with Warp.

Backward Compatibility should be a must for Warp and Enterprise or builders will not build on top of them

@llllllluc
Copy link
Contributor

llllllluc commented Nov 12, 2023

Why I usually agree adding braking changes on a product that should be the base for others, it's not the best route.

Right now we are building a grid bot on top of Warp that is getting audited.

Those changes broke our contracts on Testnet, if we'll need to develop them again we'll probably avoid Warp because the risk that braking changes will be implemented in future are too high and we surely don't want to keep changing and migrating contracts to stay up to date with Warp.

Backward Compatibility should be a must for Warp and Enterprise or builders will not build on top of them

Can you point me where your app break or describe it or paste some code? Only place i found you interact with warp is here, but this should be easily fixed and won't affect audit.

We didn't make too much change on the interface of warp, we mostly change the internal stuff and that should not affect people building on top of warp. It should be simpler to build on top of warp now, cause it remove the need to create account explicitly for new users.

I agree we should maintain backward compatibility but we need to balance it with improving warp, we can try updating feature/latest_updates to make it not break your app. In the future, i don't think we will make structural change like this one again.

@0x7183
Copy link

0x7183 commented Nov 12, 2023

The hackathon repo is not updated, right now we are creating the job for the users directly inside the contracts, to ensure a better user experience.

Contracts are still closed sources, but we can invite you if you want to take a look.

@llllllluc
Copy link
Contributor

The hackathon repo is not updated, right now we are creating the job for the users directly inside the contracts, to ensure a better user experience.

Contracts are still closed sources, but we can invite you if you want to take a look.

Ok please invite me, thanks!

@0x7183
Copy link

0x7183 commented Nov 12, 2023

@llllllluc invited

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.

3 participants