-
Notifications
You must be signed in to change notification settings - Fork 4
Polkadot v1.1.0 upgrade #4
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
Conversation
* vesting: can be used for gov * wording
* emit LockSet on extend_lock Signed-off-by: Gregory Hill <[email protected]> * emit LockSet for existing lock, add tests Signed-off-by: Gregory Hill <[email protected]> --------- Signed-off-by: Gregory Hill <[email protected]>
Signed-off-by: Gregory Hill <[email protected]>
* orml-parameters * fix * add tests * whitespaces * more tests * 0.9.43 * add weights * fix * fix * fix docs test * fix * Apply suggestions from code review Co-authored-by: Nuno Alexandre <[email protected]> * update * improve usage * fix * update comment * set weight --------- Co-authored-by: Nuno Alexandre <[email protected]>
ce621b4 to
a7a0861
Compare
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 see a bit of inconsistency related to Cargo.toml files, dependencies and branches. In some places v1.0.0 is used and in some others v1.1.0. We should use v1.1.0 in all packages, and dependencies and commit branches should be consistent.
.gitignore
Outdated
| .DS_Store | ||
| .idea | ||
|
|
||
| .vscode |
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 should avoid changing this kind of files. It is recommended to strictly change the necessary files associated to the update (Cargo.toml ones and those that require upstream changes).
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.
done 👍
Cargo.dev.toml
Outdated
| sp-tracing = { git = "https://github.com/moonbeam-foundation//polkadot-sdk", branch = "cem-upgrade-to-polkadot-1.1.0" } | ||
| sp-wasm-interface = { git = "https://github.com/moonbeam-foundation//polkadot-sdk", branch = "cem-upgrade-to-polkadot-1.1.0" } | ||
| sp-io = { git = "https://github.com/moonbeam-foundation//polkadot-sdk", branch = "cem-upgrade-to-polkadot-1.1.0" } | ||
| sp-keystore = { git = "https://github.com/moonbeam-foundation//polkadot-sdk", branch = "cem-upgrade-to-polkadot-1.1.0" } |
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 see that a lot of dependencies in this file are duplicated. You should only import them once. Also, I think the patches to the different repos (paritytech/cumulus, paritytech/polkadot, paritytech/substrate) are not needed anymore since they were all merged into the mono-repo. In that case, you should have one patch for the entire repo.
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.
Pr is not finalized yet.
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.
done
The PR is just a draft, not finalized. I have pushed so that you can see the failing tests. Also, only the patch section has been modified to point to new polkadot-sdk repo, which is enough for me to proceed right now. |
|
That's okay, I was pointing that there are duplicated dependencies because that might be the reason why some tests are failing, but will take a look at them now. |
|
Also @rimbi, can you please indicate which tests are failing? Thanks :) Edit: I already saw that are the ones related to xtokens. |
|
After diving into the code, and as I saw while debugging the tests, it seems that there are not XCM upstream related changes that may affect the behavior of the xtokens tests. I think the problem could be associated to the fact that there is one commit from upstream master branch that was never cherry-picked to our As these tests are all passing on upstream master branch and there don't seem to be XCM related changes that may have changed the behavior, I would say almost for sure that the failures come due to a misconfiguration regarding this inconsistency between branches. What you can do as a next step is to cherry-pick the missing commits into our Hope this helps! Let me know when you make more progress on it, I'll be glad to give a second look and help with tests :) Edit for visibility: Indeed there is a change included upstream inside |
Thank you very much for the effort, Agustin 🙏 Checking it... Looks like the changes in the |
* update gh action concurrency * update
a8425cc to
9998244
Compare
Agusrodri
left a comment
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.
The changes look good to me so far. We should pay attention to how upstream ORML will manage the case of the new restriction in the barrier (you can also open a PR upstream proposing to upgrade master with your changes).
9998244 to
f945515
Compare
Makes sense! Here is the PR. 👍 |
55b4b6b to
fdced49
Compare
|
I am closing this PR and will open another one. The reason is that I have some updates in the base branch (which does not effect the current change set) but github does not take that into account and not recalculate the diffs. |
This is the PR upgrade dependencies to polkadot v1.1.0.