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

Better error message with missing sudo (no parse error) #279

Merged
merged 1 commit into from
Apr 21, 2021

Conversation

ethanfrey
Copy link
Member

Closes #278

I introduced Any rather than String. Any is like Empty (parses any struct), but it has a needed Display implementation.
When we are happy with this, and merged, we can port over the Display implementation to cosmwasm_std::Empty for the future.

@ethanfrey ethanfrey requested a review from maurolacy April 20, 2021 20:30
packages/multi-test/src/wasm.rs Show resolved Hide resolved
@ethanfrey ethanfrey merged commit 46c8d69 into master Apr 21, 2021
@ethanfrey ethanfrey deleted the better-sudo-error-messages branch April 21, 2021 08:41
@@ -48,7 +60,7 @@ type QueryClosure<T, E> = Box<dyn Fn(Deps, Env, T) -> Result<Binary, E>>;

/// Wraps the exported functions from a contract and provides the normalized format
/// Place T4 and E4 at the end, as we just want default placeholders for most contracts that don't have sudo
pub struct ContractWrapper<T1, T2, T3, E1, E2, E3, C = Empty, T4 = String, E4 = String>
pub struct ContractWrapper<T1, T2, T3, E1, E2, E3, C = Empty, T4 = Any, E4 = Any>
Copy link
Member

@webmaster128 webmaster128 Apr 21, 2021

Choose a reason for hiding this comment

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

Why don't you use T4 = Empty and E4 = String?

String is a great default error type. The only restrictions those types have is ToString. So requiring Display is more than necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm.. that might work and be a simpler solution.
Will try

Copy link
Member Author

Choose a reason for hiding this comment

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

Just approved your PR to that end

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.

Sudo over no new_with_sudo contract wrapper error message
3 participants