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

chore: upgrade to 1.8.0 #156

Conversation

chungquantin
Copy link
Collaborator

@chungquantin chungquantin commented Aug 4, 2024

CHANGELOG

Contracts-Related Changes

3384: pallet_contracts stabilize call_v2, instantiate_v2, lock_dependency and unlock_dependency.

These APIs are currently unstable and are being stabilized in this PR.
Note: add_delegate_dependency and remove_delegate_dependency have been renamed to lock_dependency and unlock_dependency respectively.

3243: Don't fail fast if the weight limit of a cross contract call is too big

Cross contracts calls will now be executed even if the supplied weight limit is bigger than the reamining weight. If the actual weight is too low they will fail in the cross contract call and roll back. This is different from the old behaviour where the limit for the cross contract call must be smaller than the remaining weight.

3154: Contracts: Stabilize caller_is_root API

Removed the #[unstable] attrribute on caller_is_root host function.

3184: Contracts: Remove no longer enforced limits from the Schedule

The limits are no longer in use and do nothing. Every builder overwritting them can just adapt their code to remove them without any consequence.

3415: [pallet-contracts] Add APIVersion to the config.

Add APIVersion to the config to communicate the state of the Host functions exposed by the pallet.

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 3.30%. Comparing base (95bb4b5) to head (dc808e4).

Files Patch % Lines
node/src/command.rs 0.00% 1 Missing ⚠️
@@           Coverage Diff            @@
##            main    #156      +/-   ##
========================================
- Coverage   3.30%   3.30%   -0.01%     
========================================
  Files         30      30              
  Lines       6843    6846       +3     
  Branches    6843    6846       +3     
========================================
  Hits         226     226              
- Misses      6617    6620       +3     
Files Coverage Δ
node/src/service.rs 0.00% <ø> (ø)
runtime/devnet/src/config/contracts.rs 60.00% <ø> (ø)
runtime/testnet/src/config/contracts.rs 60.00% <ø> (ø)
node/src/command.rs 0.00% <0.00%> (ø)

@evilrobot-01
Copy link
Collaborator

Awesome! I especially like the changelog. 🔥

Shall we perhaps include a subsection in the changelog specifically around contracts, with links to anything that is relevant to contracts in the release notes/prdocs? This is simply to save any reviewers some time, as the person doing the upgrade would have covered the content already, it could be greatly helpful to collate them into a section in the PR description here.

There are a lot of improvements in the contracts pallet in coming releases so would be ideal that we are all familiar with them. and their effects.

@chungquantin
Copy link
Collaborator Author

chungquantin commented Aug 4, 2024

@evilrobot-01 I have updated the PR to cover contracts related changes. However, seems like this upgrade made changes to the XCM integration tests, failed with different events returned. I am taking a look to find commits that leads to this.

[
+    RuntimeEvent::System(
+        Event::KilledAccount {
+           account: 7061726182230000000000000000000000000000000000000000000000000000 (5Ec4AhPD...),
+        },
+    ),
+    RuntimeEvent::Balances(
+        Event::DustLost {
+           account: 7061726182230000000000000000000000000000000000000000000000000000 (5Ec4AhPD...),
+           amount: 400000,
+      },
+    ),
+    RuntimeEvent::Balances(
+       Event::Burned {
+            who: 7061726182230000000000000000000000000000000000000000000000000000 (5Ec4AhPD...),
+           amount: 33332933000,
+        },
+    ),
+    RuntimeEvent::Balances(
+        Event::Minted {
+            who: 8eaf04151687736326c9fea17e25fc5287613693c912909cb226aa4794f26a48 (5FHneW46...),
+            amount: 33322539492,
+        },
+    ),
    RuntimeEvent::MessageQueue(
        Event::Processed {
            id: 0x1d61b5bc4d5bb97434afc46b0a8bba08041dbb4eb660a100da5613031a471f11,
            origin: AggregateMessageOrigin::Ump(
                UmpQueueId::Para(
                    Id(
                        9090,
                    ),
                ),
            ),
            weight_used: Weight {
                ref_time: 306305000,
                proof_size: 7186,
            },
            success: true,
        },
    ),
]

@evilrobot-01
Copy link
Collaborator

I think this is due to the change from currency adapter to fungibles adapter.

See my other open PR which updates the integration tests to use Paseo rather than Rococo.

@chungquantin chungquantin self-assigned this Aug 4, 2024
@chungquantin chungquantin linked an issue Aug 4, 2024 that may be closed by this pull request
@peterwht
Copy link
Collaborator

peterwht commented Aug 7, 2024

Just commenting as a useful issue for this PR: moondance-labs/tanssi#574

Copy link
Collaborator

@peterwht peterwht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Just needs the resolved tests from Frank's PR.

fyi for other reviewers:

  • did a diff of parachain template service.rs, no differences relevant to this upgrade.

@evilrobot-01
Copy link
Collaborator

Expect there will be compilation issues here when the merge conflicts are resolved.

I pointed out that the integration tests PR switching to Paseo runtimes meant we would be stuck on SDK 1.7, as that is what Paseo is on. Emulated tests compile into a single build artifact and therefore need the same versions.

Only other option is using the Polkadot runtime instead, as Paseo is based on it, but that would require using a specific commit there and a jump to 1.13. I see they are now on 15, at least on main. Last release is still pre 1.13.

@evilrobot-01
Copy link
Collaborator

My suggestion would be to first get async backing enable on the testnet, then upgrade to 1.13 before updating collators and updating runtime. These can be done in parallel.

A decision would also need to be made on how to handle the update. One PR jumping from current to .13 or several smaller PRs to branch from preceding PR to incrementally update. The latter means more smaller reviews vs one large.

Finally, the fellowship runtimes are on .15 so it could be a case of going as far as we can in one big push and then we are current and can focus elsewhere.

@peterwht
Copy link
Collaborator

peterwht commented Aug 7, 2024

My suggestion would be to first get async backing enable on the testnet, then upgrade to 1.13 before updating collators and updating runtime. These can be done in parallel.

A decision would also need to be made on how to handle the update. One PR jumping from current to .13 or several smaller PRs to branch from preceding PR to incrementally update. The latter means more smaller reviews vs one large.

Edit: Async backing and OmniNode together, then the polkadot uplift.

My recommendation would be to move to an OmniNode, then do a large jump to 1.15. Reason being that runtime changes are usually easy to review, while node changes can be a little more tedious. The current polkadot-parachain binary is async backing enabled with lookahead.

We can use Tanssi's tool to generate release changes from 1.7 -> 1.15 so it's easy to see, and also easily spot any migrations.

@Daanvdplas
Copy link
Collaborator

As talked about internally, lets create a developer branch and create all the upcoming runtime upgrade PRs to the developer branch. We are current with 1.14, we merge it as a whole to main.

@Daanvdplas
Copy link
Collaborator

This is done correct?

@chungquantin
Copy link
Collaborator Author

@Daanvdplas Should be closed because I merged it to developer branch already.

@Daanvdplas
Copy link
Collaborator

I thought so

Just checking to be sure :)

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.

chore: upgrade to polkadot 1.14
5 participants