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

progress mbm on_idle #6538

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ermalkaleci
Copy link
Contributor

@ermalkaleci ermalkaleci commented Nov 19, 2024

@ggwpez I propose we change the oder of MBM step to run on_idle so emitted event will have phase Finalization instead of ApplyExtrinsic. You already have a PR opened to change phasing but this one is more a short term solution. Let me know if this is all good so I can proceed with more tests

@ermalkaleci
Copy link
Contributor Author

Hey @ggwpez, any feedback on this

@bkchr
Copy link
Member

bkchr commented Dec 2, 2024

Are the phases such a big problem right now? Maybe we should finish this other pr by @ggwpez. Maybe you could take over this pr @ermalkaleci?

@ermalkaleci
Copy link
Contributor Author

@bkchr Yes it is a problem for chains using it. We used it on Shiden and now we have an invalid event paritytech/substrate-api-sidecar#1538
I was hoping to get it fixed first and then we can focus on a proper solution.

@ggwpez
Copy link
Member

ggwpez commented Dec 2, 2024

Can you test with this? Will try to get it through audit soon if you say that it fixes this. #3666

@ermalkaleci
Copy link
Contributor Author

Can you test with this? Will try to get it through audit soon if you say that it fixes this. #3666

yes, I did some tests and it works. I want to confirm this approach makes sense so I can write some integration tests

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.

3 participants