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

Add ibc handling to converter #64

Merged
merged 7 commits into from
Jun 23, 2023
Merged

Add ibc handling to converter #64

merged 7 commits into from
Jun 23, 2023

Conversation

ethanfrey
Copy link
Collaborator

@ethanfrey ethanfrey commented Jun 22, 2023

(Currently untested)

  • Add ibc_packet_receive with price normalization and calling the virtual staking contract.
  • Add remote denom to the instantiate entry point
  • Solve converter / virtual_staking "chicken and egg" problem (see ConverterContract::instantiate)
  • Add sudo entry point test ExecMsg variants to allow testing
  • Write simple multi-test to test this

@ethanfrey ethanfrey marked this pull request as ready for review June 22, 2023 21:46
@ethanfrey ethanfrey requested a review from maurolacy June 22, 2023 21:46
@ethanfrey
Copy link
Collaborator Author

Okay, the consumer side is implemented.

Please review, but this has some tests and should be ready to merge.
I tried doing things like:

    #[cfg(test)]
    #[msg(exec)]
    fn demo_stake(

and

    #[cfg_attr(test, msg(exec))]
    fn demo_stake(

but neither worked, as #[msg] doesn't seem to be an attribute but is actually parsed strings by the #[contract] macro.
If you find/know a better way to conditionally compile these methods, only for testing, let me know.

I am happy to use a feature flag as well, but the main part is some sort of cfg directive that plays well with the msg(exec) directive

Copy link
Collaborator

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Looks good! Will take a look at the tests next.

contracts/consumer/converter/src/ibc.rs Show resolved Hide resolved
contracts/consumer/converter/src/contract.rs Outdated Show resolved Hide resolved
contracts/consumer/converter/src/contract.rs Outdated Show resolved Hide resolved
.add_attribute("validator", &validator)
.add_attribute("amount", amount.amount.to_string());

let msg = virtual_staking_api::ExecMsg::Bond { validator, amount };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Shouldn't this better be called Delegate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question, can you make a different issue on this?
Out of scope of the IBC connection

contracts/consumer/converter/src/contract.rs Outdated Show resolved Hide resolved
use price_feed_api::Querier;
let remote = price_feed_api::Remote::new(config.price_feed);
let price_feed = remote.querier(&deps.querier);
let price = price_feed.price()?.native_per_foreign;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Guess this query will look something like

let price = price_feed.price(config.remote_denom)?.native_per_foreign;

in the (near?) future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, related issue: CosmWasm/sylvia#181
I will add this to code comment

contracts/consumer/converter/src/contract.rs Outdated Show resolved Hide resolved
@maurolacy
Copy link
Collaborator

maurolacy commented Jun 23, 2023

neither worked, as #[msg] doesn't seem to be an attribute but is actually parsed strings by the #[contract] macro. If you find/know a better way to conditionally compile these methods, only for testing, let me know.

I am happy to use a feature flag as well, but the main part is some sort of cfg directive that plays well with the msg(exec) directive

Was trying to do the same to no avail. I think we need to use some form of sudo. The trick with the "ADMIN" check is something I thought but decided not to do. Will work on improving this asap.

@ethanfrey
Copy link
Collaborator Author

BTW, I settled on this:

#[msg(exec)]
    fn test_stake(
        &self,
        ctx: ExecCtx,
        validator: String,
        stake: Coin,
    ) -> Result<Response, ContractError> {
        #[cfg(test)] {
            // This can only ever be called in tests
            self.stake(ctx.deps, validator, stake)
        }
        #[cfg(not(test))] {
            let _ = (ctx, validator, stake);
            Err(ContractError::Unauthorized)
        }
    }

The entry point is still there, but it is compile-time fixed to errors in non-test builds.

@ethanfrey ethanfrey changed the base branch from external-staking-locks-2 to main June 23, 2023 08:17
@ethanfrey ethanfrey merged commit b765ebd into main Jun 23, 2023
@ethanfrey ethanfrey deleted the consumer-side-ibc-staking branch June 23, 2023 08:22
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