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

Keep im-online events decodable after pallet removal #235

Conversation

s0me0ne-unkn0wn
Copy link
Contributor

@s0me0ne-unkn0wn s0me0ne-unkn0wn commented Mar 12, 2024

This is a follow-up to #178, which aims to keep the im-online pallet events decodable after the pallet itself is removed.

As discussed, the inability to decode events may result in missing a candidate in approval voting and a parachain finality stall. Although the circumstances of such an event are unlikely, it's better to safeguard them.

The transient code introduced in this PR should be removed after the upgrade is enacted.

  • Does not require a CHANGELOG entry

Copy link
Contributor

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

Looks sensible to me, assuming it was tested to actually work. (E.g. does the pallet path matter?)

@s0me0ne-unkn0wn
Copy link
Contributor Author

assuming it was tested to actually work

Yes, I reproduced the problem in a local synthetic test and made sure it was resolved with this patch. Paths do not matter, TypeInfo only includes names, I'm not sure it's even critical in this case, but I still named the thing pallet_im_online to be on the safe side.

@s0me0ne-unkn0wn s0me0ne-unkn0wn mentioned this pull request Mar 15, 2024
19 tasks
Copy link
Contributor

@ordian ordian left a comment

Choose a reason for hiding this comment

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

This is a reasonable workaround. I assume we can safely remove this mock pallet after the runtime upgrade is applied (let's create an issue to not forget)?

@eskimor
Copy link
Contributor

eskimor commented Mar 15, 2024

/merge

@fellowship-merge-bot fellowship-merge-bot bot enabled auto-merge (squash) March 15, 2024 11:32
@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

@fellowship-merge-bot fellowship-merge-bot bot merged commit 64c9e16 into polkadot-fellows:main Mar 15, 2024
29 of 30 checks passed
@s0me0ne-unkn0wn
Copy link
Contributor Author

I assume we can safely remove this mock pallet after the runtime upgrade is applied (let's create an issue to not forget)?

Fair enough, raised #242

@s0me0ne-unkn0wn s0me0ne-unkn0wn deleted the remove-im-online-fix-events branch March 15, 2024 20:39
@bkchr
Copy link
Contributor

bkchr commented Mar 17, 2024

Thank you! Good approach!

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.

4 participants