Skip to content

fix: remove staking tokens from add validator event#139

Open
kpitapeersyst wants to merge 3 commits into
mainfrom
fix/poa-add-validator-event-tokens
Open

fix: remove staking tokens from add validator event#139
kpitapeersyst wants to merge 3 commits into
mainfrom
fix/poa-add-validator-event-tokens

Conversation

@kpitapeersyst

@kpitapeersyst kpitapeersyst commented May 4, 2026

Copy link
Copy Markdown
Contributor

fix: remove staking tokens from add validator event

Motivation

  • The add_validator event should not expose staking token amounts from the staking keeper.
  • Removing staking_tokens avoids coupling this POA event to an extra post-create validator lookup.

Changes

  • Remove AttributeStakingTokens from ExecuteAddValidator event emission.

@coderabbitai

coderabbitai Bot commented May 4, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

ExecuteAddValidator re-fetches the staking validator after the MsgCreateValidator handler completes and updates the EventTypeAddValidator emission to report the post-creation staking token amount instead of the pre-creation value.

Changes

Post-creation validator token fix

Layer / File(s) Summary
Re-query and emit post-creation staking tokens
x/poa/keeper/keeper.go
After the staking keeper's MsgCreateValidator handler completes, GetValidator is called again to fetch the created validator, and EventTypeAddValidator now emits AttributeStakingTokens using the post-creation validator.Tokens value.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

A rabbit hops through keeper code,
With tokens fresh from creation's mode,
The query runs, the event does sing,
Post-creation brings truth to the ring! 🐰✨

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The PR description is incomplete and contradicts the actual changes. It says to 'remove' AttributeStakingTokens, but the objectives show the change should 'emit' post-create validator tokens. Update the description to accurately reflect the actual changes: re-fetch validator after MsgCreateValidator and emit AttributeStakingTokens with post-create token values.
Title check ❓ Inconclusive The title is partially related to the changeset. It mentions removing staking tokens from the event, but the actual change is about emitting the correct post-creation token value, not removing tokens entirely. Clarify the title to better reflect that staking tokens are being emitted with the post-creation validator state, not removed.
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/poa-add-validator-event-tokens

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kpitapeersyst kpitapeersyst force-pushed the fix/poa-add-validator-event-tokens branch from 1b853c5 to 83a8653 Compare May 4, 2026 18:05

@AdriaCarrera AdriaCarrera left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's better to remove the event attribute itself

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
x/poa/keeper/keeper_test.go (1)

418-423: ⚡ Quick win

Avoid asserting on the last event index directly.

Using events[len(events)-1] makes this test fragile if any later event is added. Search for types.EventTypeAddValidator in the emitted events and assert on that match instead.

Suggested test hardening
-	event := events[len(events)-1]
-	require.Equal(t, types.EventTypeAddValidator, event.Type)
-
-	attribute, ok := event.GetAttribute(types.AttributeStakingTokens)
+	var event sdk.Event
+	found := false
+	for i := len(events) - 1; i >= 0; i-- {
+		if events[i].Type == types.EventTypeAddValidator {
+			event = events[i]
+			found = true
+			break
+		}
+	}
+	require.True(t, found, "add_validator event not found")
+
+	attribute, ok := event.GetAttribute(types.AttributeStakingTokens)
 	require.True(t, ok)
 	require.Equal(t, fmt.Sprintf("%d", sdk.DefaultPowerReduction), attribute.Value)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@x/poa/keeper/keeper_test.go` around lines 418 - 423, The test currently
assumes the EventTypeAddValidator is the last emitted event by using
events[len(events)-1]; instead, iterate or search the events slice for an event
with Type == types.EventTypeAddValidator (e.g., loop over events or use a helper
to find the first matching event), assert that such an event exists, then call
event.GetAttribute(types.AttributeStakingTokens) and check the attribute.Value
equals fmt.Sprintf("%d", sdk.DefaultPowerReduction); update the assertions in
keeper_test.go to reference the found event rather than the last index to avoid
fragility.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@x/poa/keeper/keeper_test.go`:
- Around line 418-423: The test currently assumes the EventTypeAddValidator is
the last emitted event by using events[len(events)-1]; instead, iterate or
search the events slice for an event with Type == types.EventTypeAddValidator
(e.g., loop over events or use a helper to find the first matching event),
assert that such an event exists, then call
event.GetAttribute(types.AttributeStakingTokens) and check the attribute.Value
equals fmt.Sprintf("%d", sdk.DefaultPowerReduction); update the assertions in
keeper_test.go to reference the found event rather than the last index to avoid
fragility.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5655b91c-1fb1-4f71-ac78-f3a5ddb7fccd

📥 Commits

Reviewing files that changed from the base of the PR and between 83a8653 and 74715b0.

📒 Files selected for processing (1)
  • x/poa/keeper/keeper_test.go

@kpitapeersyst kpitapeersyst changed the title fix: poa emit post-create validator tokens fix: remove staking tokens from add validator event Jun 9, 2026
@kpitapeersyst

Copy link
Copy Markdown
Contributor Author

CI failure is unrelated to this PR, will be resolved once #143 is merged.

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