[CORE-708] Explicitly log out delayed message when execution fails #687
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe changes primarily focus on enhancing the logging capabilities in the Changes
TipsChat with CodeRabbit Bot (
|
teddyding
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/delaymsg/keeper/dispatch.go (1 hunks)
- protocol/x/delaymsg/types/keys.go (1 hunks)
Additional comments: 2
protocol/x/delaymsg/types/keys.go (1)
- 26-28: The addition of
MessageContentLogKeyandMessageTypeUrlLogKeyis a good move for enhancing the logging capabilities. However, ensure that the values assigned to these constants are not sensitive or private data that should not be logged. Also, consider the size of the message content, as logging large messages could impact performance or storage.protocol/x/delaymsg/keeper/dispatch.go (1)
- 54-64: The new logging statement provides more context when a delayed message fails to execute by including the message content and type URL in the error log. This is a good practice as it will help in debugging and troubleshooting. However, be aware that logging the entire message content might include sensitive data. If the messages can contain sensitive data, consider sanitizing or hashing the content before logging.
- k.Logger(ctx).Error("failed to execute delayed message", types.IdLogKey, id, constants.ErrorLogKey, err) + k.Logger(ctx).Error( + "failed to execute delayed message", + types.IdLogKey, + id, + constants.ErrorLogKey, + err, + types.MessageContentLogKey, + sanitize(msg.String()), // Ensure sensitive data is not logged + types.MessageTypeUrlLogKey, + delayedMsg.Msg.TypeUrl, + )
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/testutil/constants/bridge.go (2 hunks)
- protocol/x/bridge/app_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/bridge/app_test.go
Additional comments: 2
protocol/testutil/constants/bridge.go (2)
36-39: The
emptyCoinvariable is introduced with a denomination of "adv4tnt" and an amount of 0. Ensure that this denomination is valid and that a coin with an amount of 0 is handled correctly in all places whereemptyCoinis used.73-78: A new
BridgeEventis introduced with an ID of 0, the address of Alice, theemptyCoinas the coin, and an Ethereum block height of 0. Ensure that these values are valid and handled correctly in all places whereBridgeEvent_Id4_Height0_EmptyCoinis used. In particular, verify that aBridgeEventwith an ID of 0 and an Ethereum block height of 0 is acceptable and does not conflict with any existing events.
teddyding
changed the title
CORE-708 In `delaymdg`, explicitly log out message that failed execution
I can take this one since crystal has quite a few other tasks here. |
tqin7
approved these changes
Oct 24, 2023
protocol/x/bridge/app_test.go
Outdated
| @@ -72,6 +72,23 @@ func TestBridge_Success(t *testing.T) { | |||
| blockTime: time.Now(), | |||
| expectNonEmptyBridgeTx: true, | |||
| }, | |||
| "Success: 1 bridge event with 0 coin denom, delay 5 blocks": { | |||
There was a problem hiding this comment.
nit:
Suggested change
| "Success: 1 bridge event with 0 coin denom, delay 5 blocks": { | |
| "Success: 1 bridge event with 0 coin amount, delay 5 blocks": { |
clemire
approved these changes
Oct 24, 2023
ttl33
reviewed
Oct 24, 2023
protocol/x/delaymsg/types/keys.go
Outdated
| @@ -23,5 +23,7 @@ const ( | |||
|
|
|||
| // Log | |||
| const ( | |||
There was a problem hiding this comment.
nit: we should prob create a separate file types/constants.go or types/logs.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/bridge/app_test.go (1 hunks)
- protocol/x/delaymsg/types/keys.go (1 hunks)
- protocol/x/delaymsg/types/logs.go (1 hunks)
Files skipped from review due to trivial changes (3)
- protocol/x/bridge/app_test.go
- protocol/x/delaymsg/types/keys.go
- protocol/x/delaymsg/types/logs.go
|
@Mergifyio backport release/protocol/v1.x |
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
Oct 24, 2023
) * explicitly log out message when dispatch fails * add e2e test case where delayed MsgCompleteBridge fails to execute * nits (cherry picked from commit 1f065b4)
teddyding
added a commit
that referenced
this pull request
Oct 24, 2023
) (#696) * explicitly log out message when dispatch fails * add e2e test case where delayed MsgCompleteBridge fails to execute * nits (cherry picked from commit 1f065b4) Co-authored-by: Teddy Ding <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelist
Test Plan
Added e2e test where a BridgeEvent contains
0adv4tnttransfer. The error log shows up as following:Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.