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

Emit redacted errors #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Emit redacted errors #17

wants to merge 1 commit into from

Conversation

nicolaslara
Copy link

Initial implementation of CosmWasm#1122

@@ -3,6 +3,7 @@
import (
"bytes"
"fmt"
"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism
}

event := sdk.NewEvent(types.CustomContractEventPrefix+types.EventTypeRedactedError, attrs...)
ctx.EventManager().EmitEvent(event)
Copy link

Choose a reason for hiding this comment

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

Please note that this is a cached context. In the SDK events are only emitted when the TX succeeds. There is no guarantee that this is the case.
The contract can decide to fail the TX on reply and rollback. When the contract handles the error reply, is there still value in the event?

Copy link
Author

@nicolaslara nicolaslara May 11, 2023

Choose a reason for hiding this comment

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

Ahh good catch! I'll make sure those events are copied to the parent when the context is discarded.

It's also a good question what we want to happen if the failed message is caught by the contract. I started writing this to be able to provide feedback when a transaction does ultimately fails, so I think it's ok for the events to be omitted if the contract catches the failure, as I assume that means the contract is properly handling the errors.

Do you think there is value in surfacing those errors anyway?

Copy link
Author

Choose a reason for hiding this comment

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

ahh, this is annoying: this would have to be done in baseapp:runTx. Trying to figure out a good workaround

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