-
Notifications
You must be signed in to change notification settings - Fork 51
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
UPdate 1.11.0 #561
UPdate 1.11.0 #561
Conversation
Signed-off-by: girazoki <gorka.irazoki@gmail.com>
* Add migration for ForeignAssetCreator * use accurate weight
{ | ||
// this seems to be called for substrate-based transactions | ||
fn on_unbalanceds<B>(mut fees_then_tips: impl Iterator<Item = NegativeImbalance<R>>) { |
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.
This seems to be typo.
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.
which part? I think it just made the line bigger and therefore fmt has decided to put it this way?
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.
on_unbalanceds
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 trait so if there is a typo, its in parity
@@ -35,10 +37,6 @@ | |||
{ | |||
"name": "charlie", | |||
"validator": true |
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 did we remove 4th node?
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.
Because we only need 3 validators in reality (actually it might have just work with 2)
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.
We have only dancebox + one container, so I think we can reduce it to 2. Do you want me to try this?
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.
Generally, I am in favor of reducing complexity of test setup. So, yeah we can reduce it to 2. But, I would suggest to do it in another PR as this PR is already too big. :)
concrete: { | ||
parents: 2, | ||
interior: { Here: null }, | ||
}, |
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 fungible assets were removed from allowed assets?
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.
I think it is because now they are only identified by their Location only
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.
Actually I think this test was wrong before, I am not sure why it was working. According to my knowledge the right type is a multilocation. @fgamundi do you know why we put a fungible there¿?
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.
Not sure why fungible was there. The right type is indeed a MultiLocation
(AssetId
, really). Must have gotten confused with the actual XcmFragment
s in the test.
I just did some tests and seems like Polkadot.js just ignores any additional data that doesn't belong to the correct type, ignoring fun
in this case
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.
Few questions. But apart from that looks good.
TODOs:
By changing the
DealWithFees
struct (we were forced to) we now are losing the treasury.Deposit event (as we are using ResolveTo instead, something that also parity does;https://github.com/paritytech/polkadot-sdk/blob/4ab078d6754147ce731523292dd1882f8a7b5775/polkadot/runtime/common/src/impls.rs#L64. We should investigate if there is a way of maintaining still that event, although it is redundant (and always has be) because the balances.Deposit event as wellWrite migrations for pallets
ForeignAssetCreator
, where we should migrate keys from V3 to V4.Include migrations to execute on xcm-executor-utils & pallet-xcm
Fix all ts tests