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

Sudo entry point #433

Merged
merged 5 commits into from
Mar 4, 2021
Merged

Sudo entry point #433

merged 5 commits into from
Mar 4, 2021

Conversation

ethanfrey
Copy link
Member

Closes #420

Merge after #432 (this is based on that)

Note the entry point name changed from system -> sudo after some discussions in CosmWasm

@ethanfrey ethanfrey requested a review from alpe as a code owner March 3, 2021 19:50
@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #433 (311be37) into master (4307e9b) will increase coverage by 0.13%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #433      +/-   ##
==========================================
+ Coverage   55.34%   55.47%   +0.13%     
==========================================
  Files          39       39              
  Lines        4022     4043      +21     
==========================================
+ Hits         2226     2243      +17     
- Misses       1616     1618       +2     
- Partials      180      182       +2     
Impacted Files Coverage Δ
x/wasm/internal/keeper/keeper.go 87.95% <71.42%> (-0.41%) ⬇️

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Very nice 👍 . Only minor nit on the test example


// now the community wants to get paid via sudo
msg := sudoMsg{
StealFunds: stealFundsMsg{
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is the best use case of this function 🤣

👮👮👮 But more serious, people tend to not understand jokes very well when it comes to money or security. Maybe some additional context can help tot explain that this is not about stealing money but calling a privileged function in the contract that would otherwise not be available. This example would not work with random contract but only with the hackatom example that had this stealFundsMsg function added for this particular demo case. We do not encourage people to build backdoors into their contracts... 👮 👮👮

🤣 🤣 🤣

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add a comment. I added it in CosmWasm as a funny example.
Our first tutorial of cosmwasm was to modify the escrow contract to add a backdoor. And then said this is why it is essential you verify the actual rust code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rebased and added a comment in 311be37

@ethanfrey ethanfrey merged commit b09a925 into master Mar 4, 2021
@ethanfrey ethanfrey deleted the sudo-entry-point branch March 4, 2021 10:09
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.

Expose new "system" entry point for contracts
2 participants