-
Notifications
You must be signed in to change notification settings - Fork 49
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
To polkadot-v1.7.2
#1516
To polkadot-v1.7.2
#1516
Conversation
Signed-off-by: Xavier Lau <xavier@inv.cafe>
Signed-off-by: Xavier Lau <xavier@inv.cafe>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Check 3a52562
|
// BridgeKusamaParachain: pallet_bridge_parachains::<Instance1> = 40, | ||
// BridgeCrabMessages: pallet_bridge_messages::<Instance1> = 41, | ||
// BridgeCrabDispatch: pallet_bridge_dispatch::<Instance1> = 42, | ||
// CrabFeeMarket: pallet_fee_market::<Instance1> = 43 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not keep these comment and give pallet_message_queue
a new index 44?
Better safe than sorry, even if the storage of these modules hava been cleared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only runtime call cares about the index. These won't affect the storage. I think there isn't security issue. Unless there are some on-going scheduled call. But those pallets were removed long time ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hackfisher We have some dissicussion internally, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both options look good to me. I will probably keep them to ensure we maintain good co-coding conventions, as I will not be able to recall why we skipped these indexes in future. Additionally, This encourage others not skipping without comments as we was doing it, new contributors usually following existing code practices.
However, I do not have reasons to argue against it from a security perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a super huge pull request, it took me the whole afternoon to review the changes. I leave some feedback. Most of the changes look good to me. You removed two pallets, and there are also some tasks left to do or migrations needed, such as the asset description migration for the Koi testnet. I assume there will be another PR to handle the migration.
Yep. |
No description provided.