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.

Sign up for GitHub

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

Jump to bottom

[xcm-builder] Replaced deprecated CurrencyAdapter with FungibleAdapter #3287

Merged
merged 9 commits into from
Feb 13, 2024
Merged

[xcm-builder] Replaced deprecated CurrencyAdapter with FungibleAdapter #3287

merged 9 commits into from
Feb 13, 2024

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Feb 12, 2024 •
edited
Loading

I found out during the cleanup of this deprecation message in the polkadot-fellows repository that we deprecated CurrencyAdapter without making the recommended changes.

TODO

@bkontur bkontur added R0-silent Changes should not be mentioned in any release notes T6-XCM This PR/Issue is related to XCM. labels Feb 12, 2024
@bkontur bkontur requested review from athei and a team as code owners February 12, 2024 09:22
@bkontur
Copy link
Contributor Author

bkontur commented Feb 12, 2024

@franciscoaguirre I see that tests/benchmarks fails, also I am checking your PR: #2684

and I'll change it to the new FungibleAdapter in a following PR.
The two stages are separated so as to not bloat this PR with some name fixes in tests.

What about that following PR? :)

also I changed CurencyAdapter to FungibleAdapter for fellows and tests started to fail: https://github.com/polkadot-fellows/runtimes/actions/runs/7869327889/job/21468225062?pr=159

@bkontur bkontur mentioned this pull request Feb 12, 2024
Merged
11 tasks
@franciscoaguirre
Copy link
Contributor

Pallet balances' fungible implementation emits different events than its Currency implementation. I had to change them in the tests to make them pass

@franciscoaguirre
Copy link
Contributor

Need to change all the other runtimes

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5195504

@franciscoaguirre
Copy link
Contributor

Seems there's a problem with benchmarks... Looking into it

@franciscoaguirre
fix(pallet-xcm-benchmarks): Changed assertion in transfer_asset ben…
62474e6 
…chmark

The `CurrencyAdapter` allowed transferring even when it would destroy the account,
because of having less than the ED.
The new `FungibleAdapter` always tries to preserve the sending account.
This benchmark was leaving an account with zero after transferring, which is why
it was working before but not after the change of adapter.
@franciscoaguirre
fix(snowbridge): Change expected error message in test
2c3da47
KiChjang
KiChjang reviewed Feb 13, 2024
View reviewed changes
bridges/snowbridge/runtime/test-common/src/lib.rs
@@ -188,6 +188,7 @@ pub fn send_transfer_token_message_success<Runtime, XcmConfig>(
let channel_id: ChannelId = origin.into();

let nonce = snowbridge_pallet_outbound_queue::Nonce::<Runtime>::try_get(channel_id);
dbg!(&nonce);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @claravanstaden

seadanda
seadanda approved these changes Feb 13, 2024
View reviewed changes
polkadot/xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs
@@ -79,7 +82,7 @@ benchmarks_instance_pallet! {
}: {
executor.bench_process(xcm)?;
} verify {
assert!(T::TransactAsset::balance(&sender_account).is_zero());
assert!(T::TransactAsset::balance(&sender_account) < sender_account_balance_before);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a tight enough bound on this test? It's just checking that it decreases by any amount, whereas do we not expect it to change by some predictable amount?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it should be tighter. Will look into it on another PR 😄

@franciscoaguirre franciscoaguirre added this pull request to the merge queue Feb 13, 2024
Merged via the queue into master with commit e0c902e Feb 13, 2024
128 of 130 checks passed
@franciscoaguirre franciscoaguirre deleted the bko-replaced-deprecated-currency-adapter branch February 13, 2024 19:52
@franciscoaguirre franciscoaguirre mentioned this pull request Feb 14, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Feb 14, 2024
@franciscoaguirre
Remove forgotten dbg! in a snowbridge test (#3322)
15103d7 
Forgotten in #3287 😅
bkontur added a commit that referenced this pull request Feb 21, 2024
@bkontur @franciscoaguirre
[xcm-builder] Replaced deprecated CurrencyAdapter with FungibleAdapter (
b0b7472 
#3287)

I found out during the cleanup of this deprecation message in the
`polkadot-fellows` repository that we deprecated `CurrencyAdapter`
without making the recommended changes.


## TODO
- [ ] fix `polkadot-fellows` bump to 1.6.0
polkadot-fellows/runtimes#159

---------

Co-authored-by: Francisco Aguirre <[email protected]>
@bkontur bkontur mentioned this pull request Feb 21, 2024
Merged
acatangiu pushed a commit that referenced this pull request Feb 21, 2024
@bkontur @franciscoaguirre
Backport patch for xcm fungible benchmarks required by new FungibleAd…
a2da02b 
…apter (#3417)

We replaced deprecated `CurrencyAdapter`
[here](#3287) and we also
did the same for fellows
[here](polkadot-fellows/runtimes@2d6d3ac),
but this change requires this patch for xcm benchmarks.

Expected patch for  `pallet-xcm-benchmarks` as `8.0.1`

Co-authored-by: Francisco Aguirre <[email protected]>
@ghost ghost mentioned this pull request Feb 26, 2024
Closed
bkchr pushed a commit to polkadot-fellows/runtimes that referenced this pull request Feb 27, 2024
@bkontur @joepetrowski
@franciscoaguirre @brenzi @acatangiu @fellowship-merge-bot
Upgrade to latest [email protected] release (#159)
59172d5 
Based on bump to
[`[email protected]`](#137).

Attached result of `cargo upgrade -v --pinned --incompatible`
[cargo-upgrade-version-bump.log](https://github.com/polkadot-fellows/runtimes/files/14044160/cargo-upgrade-version-bump.log)

_Note: Encointer was not upgraded (because its pallet references
`[email protected]` release)._

## ~~For reviewers~~

~~This PR is against `polkadot-fellows`'s main to bring it to the
fellows repo, but if you want to see a real diff relevant to the
`[email protected]` update please check:
bkontur/runtimes@bko-bump-to-1.5...bkontur:runtimes:bko-bump-to-1.6.~~


## TODO

- [x] fix compilation
- [x] fix integration tests
- [x] fix benchmarks (also try them) - `collectives-polkadot` `payout`
- [ ] ~~Does not require a CHANGELOG entry~~
- [x] `warning: use of deprecated struct
`staging_xcm_builder::CurrencyAdapter`: Use `FungibleAdapter` instead`
- [ ] search for `TODO:(PR#159) change to FungibleAdapter` and/or wait
for paritytech/polkadot-sdk#3287
- [x] patch for `pallet-nomination-pools` migration fix
paritytech/polkadot-sdk#3093
- will be fixed here
#188 (comment)
- [x] patch for `xcm-executor` fix (for 1.6.0) e.g.
paritytech/polkadot-sdk#3174
- [x] check/fix coretime stuff for Kusama/Polkadot - search for `//
TODO:(PR#159)(PR#1694)` - see
[comment](#159 (comment))
- fixed by bkontur#3
- [x] check the
`MaxControllersInDeprecationBatch`https://github.com/polkadot-fellows/runtimes/pull/159/files#r1492361038
- [x] check `pallet_identity::Config` for Kusama and Polkadot
https://github.com/polkadot-fellows/runtimes/pull/159/files#r1492363866

---------

Signed-off-by: Adrian Catangiu <[email protected]>
Co-authored-by: joe petrowski <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Ross Bulat <[email protected]>
Co-authored-by: Alain Brenzikofer <[email protected]>
Co-authored-by: eskimor <[email protected]>
Co-authored-by: Adrian Catangiu <[email protected]>
Co-authored-by: fellowship-merge-bot[bot] <151052383+fellowship-merge-bot[bot]@users.noreply.github.com>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@bkontur @franciscoaguirre
[xcm-builder] Replaced deprecated CurrencyAdapter with FungibleAdapter (
3e65142 
paritytech#3287)

I found out during the cleanup of this deprecation message in the
`polkadot-fellows` repository that we deprecated `CurrencyAdapter`
without making the recommended changes.


## TODO
- [ ] fix `polkadot-fellows` bump to 1.6.0
polkadot-fellows/runtimes#159

---------

Co-authored-by: Francisco Aguirre <[email protected]>
This was referenced Jun 5, 2024
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@franciscoaguirre franciscoaguirre franciscoaguirre left review comments

@KiChjang KiChjang KiChjang left review comments

@acatangiu acatangiu acatangiu approved these changes

@seadanda seadanda seadanda approved these changes

@athei athei Awaiting requested review from athei

Assignees

@franciscoaguirre franciscoaguirre

Labels
R0-silent Changes should not be mentioned in any release notes T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bkontur @franciscoaguirre @paritytech-cicd-pr @KiChjang @seadanda @acatangiu