chore: fix event emission for smart account module #8858
Conversation
PaddyMc
added
A:backport/v27.x
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)CHANGELOG.md (1)
The changelog entry is properly formatted and provides clear information about the fix for event emission in the smart account module. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/smart-account/ante/pubkey.go (1)
53-56: Improve error handling for address conversionThe error handling could be enhanced by:
- Using the existing context instead of returning an empty one
- Wrapping the error with additional context for better debugging
- return sdk.Context{}, err + return ctx, errorsmod.Wrapf(err, "failed to convert address bytes to string for signer %d", i)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
x/smart-account/ante/pubkey.go(1 hunks)
🔇 Additional comments (2)
x/smart-account/ante/pubkey.go (2)
51-59: LGTM - Event emission changes look correct
The changes properly address the event formatting issue by:
- Converting addresses to strings before event emission
- Maintaining consistent event attribute structure
- Following Cosmos SDK event patterns
The implementation is solid with only minor suggestions for improvement noted above.
58-59: Verify the necessity of duplicate event attributes
The code emits two nearly identical attributes with different keys:
account_sequenceaccount_sequence_authenticator
While this might be intentional for backwards compatibility, please confirm if both are necessary.
✅ Verification successful
Duplicate event attributes appear to serve different purposes
Based on the code context, the two attributes serve distinct purposes:
account_sequenceis a standard Cosmos SDK attribute key for general sequence trackingauthenticator_acc_seqis a smart-account specific attribute that appears to be part of a broader authenticator-related event system (alongside other authenticator attributes likeauthenticator_type,authenticator_id, etc.)
The duplication is justified as they serve different consumers:
- Standard SDK consumers rely on the common
account_sequenceattribute - Smart account specific features track authentication state via
authenticator_acc_seq
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for consumers of these event attributes
rg -l "account_sequence_authenticator"
rg -l "account_sequence.*EventType.*Tx"
Length of output: 80
Script:
#!/bin/bash
# Let's try a broader search for these attribute keys and their usage
rg -A 2 "AttributeKeyAccountSequence"
rg -A 2 "AttributeKeyAccountSequenceAuthenticator"
# Also search for any event handling or processing related to these attributes
rg -A 2 "HandleMessage|ProcessEvent.*account_sequence"
Length of output: 1197
Script:
#!/bin/bash
# Let's look for the context around where these attributes are used
rg -B 5 -A 5 "authenticator_acc_seq"
# Also search for any tests or documentation that might explain the purpose
fd -e md -e go | xargs rg -l "account sequence.*authenticator"
Length of output: 770
* chore: fix event emission for smart account module * chore: add changelog.md (cherry picked from commit e12cabf)
* chore: fix event emission for smart account module * chore: add changelog.md (cherry picked from commit e12cabf) Co-authored-by: PaddyMc <[email protected]>
What is the purpose of the change
Events are being formatted incorrectly when being emitted, this PR fixes that
Testing and Verifying
Review code
Query the osmosis rpc endpoints
osmosisd q tx C603B5E501D2812F888232A24403E86CCCED23AFAB8B1ADCCD21E25759B16E00 --output json --node https://rpc.osmosis.zone:443 | jqCheck these events:
run this branch:
osmosisd q tx C603B5E501D2812F888232A24403E86CCCED23AFAB8B1ADCCD21E25759B16E00 --output json | jqCheck these events:
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)