-
Notifications
You must be signed in to change notification settings - Fork 46
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
Upgrade to upstream 0.9.40 #735
Conversation
aliXsed
commented
Apr 26, 2023
•
edited
Loading
edited
- Fix xcm benchmarks for generic calls
- Use the new weight calculating functions from xcm benchmarks
- Benchmark the PR on the standard machine and update the weights
Codecov Report
@@ Coverage Diff @@
## master #735 +/- ##
===========================================
+ Coverage 44.50% 59.03% +14.53%
===========================================
Files 37 38 +1
Lines 4402 4670 +268
===========================================
+ Hits 1959 2757 +798
+ Misses 2443 1913 -530
|
@@ -34,7 +34,7 @@ use sp_runtime::{ | |||
bounded_vec, | |||
traits::{IdentifyAccount, Verify}, | |||
}; | |||
const SAFE_XCM_VERSION: u32 = xcm::v2::VERSION; | |||
const SAFE_XCM_VERSION: u32 = xcm::latest::VERSION; |
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.
wouldn't it be better to have the fixed version here to prevent future updates of xcm to be unwillingly brought to the codebase?
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.
@Hounsette The latest version of XCM would only be considered safe if we have done a full upgrade and supports that version. Bringing the safe version from a particular version was the source of some tricky upgrade issues.
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.
@aliXsed using the change below (if it is even possible) might allow us to avoid some border effects in case we have an oversight.
const SAFE_XCM_VERSION: u32 = xcm::latest::VERSION; | |
const SAFE_XCM_VERSION: u32 = xcm::v3::VERSION; |
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.
Just for reference, we discussed the cons and pros of going for the latest. Since we have had other places where the xcm definitions are fetched from the latest and we don't see any particular safety issue right now based on supporting the latest xcm that has officially brought to the scope of out code base, we decided to leave this here as it is.
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.
Some minor comments, looking good otherwise. @aliXsed do you know if we have any migrations to include?
type CallStack = [Frame<Self>; 5]; | ||
type AddressGenerator = pallet_contracts::DefaultAddressGenerator; | ||
type MaxCodeLen = ConstU32<{ 128 * 1024 }>; | ||
type MaxCodeLen = ConstU32<{ 123 * 1024 }>; |
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.
Any reasons why this is getting lowered?
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 follow the contracts system parachain and the fact that they have gone for a lower value here could be due to some safety analysis. We'd better go for the lower and increase it in the future when we have evaluated all the cons and pros.
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.
Yes of course - I was wondering as to why they did the change, not necessarily if we should do it
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.
Looks like Parity's team figured it would be appropriate to document the limits and safe values in the not so well known integrity_test
function. https://paritytech.github.io/substrate/master/src/pallet_contracts/lib.rs.html#323-386
Co-authored-by: Eliott Teissonniere <10683430+ETeissonniere@users.noreply.github.com>