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

tests: Remove cross_staking mock #114

Merged
merged 7 commits into from
Aug 31, 2023
Merged

tests: Remove cross_staking mock #114

merged 7 commits into from
Aug 31, 2023

Conversation

jawoznia
Copy link
Contributor

part of #95

@jawoznia jawoznia force-pushed the remove_cross_staking_mock branch 2 times, most recently from 0bcf1ae to 193c40d Compare August 29, 2023 14:56
Comment on lines 493 to 494
#[msg(exec)]
fn test_rollback_unstake(&self, ctx: ExecCtx, tx_id: u64) -> Result<Response, ContractError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are keeping these test helpers for now, could we move them to a separate file? They make the contracts harder to read. Ideally I would love to get rid of them, but if not a separate file would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love to get rid of them.
For now we could go with something like that: #117. Naming is bad I know.

The best workaround from sylvia side might be to add ability to conditionally use messages attribute.

Something like that would expose interface methods only if mt feature is enabled.

#[contract]
#[mt(messages(interface))]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Approach in #117 is definitely an improvement.

Love the idea of adding the ability to conditionally use messages attribute to sylvia.

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.

LGTM. Great work. Thanks for going through this.

contracts/provider/vault/src/multitest.rs Outdated Show resolved Hide resolved
contracts/provider/vault/src/multitest.rs Show resolved Hide resolved
contracts/provider/vault/src/multitest.rs Show resolved Hide resolved
contracts/provider/vault/src/multitest.rs Outdated Show resolved Hide resolved
contracts/provider/vault/src/multitest.rs Outdated Show resolved Hide resolved
contracts/provider/vault/src/multitest.rs Outdated Show resolved Hide resolved
contracts/provider/vault/src/multitest.rs Show resolved Hide resolved
@jawoznia jawoznia marked this pull request as ready for review August 31, 2023 09:29
@jawoznia jawoznia merged commit b007ced into main Aug 31, 2023
@maurolacy maurolacy deleted the remove_cross_staking_mock branch November 6, 2023 08:28
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