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

[AH] Disable Vested Transfers as preparation for AHM #579

Merged
merged 7 commits into from
Feb 7, 2025

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Feb 5, 2025

Context: #575

Nobody has used vested transfers on Polkadot or Kusama Asset Hub so far and disabling it until the Migration would avoid insertion conflicts.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez mentioned this pull request Feb 6, 2025
6 tasks
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez mentioned this pull request Feb 6, 2025
1 task
@bkchr bkchr enabled auto-merge (squash) February 7, 2025 11:30
@bkchr bkchr disabled auto-merge February 7, 2025 19:59
@bkchr bkchr merged commit 88803d5 into main Feb 7, 2025
61 of 62 checks passed
Copy link

@Grossbeatz Grossbeatz left a comment

Choose a reason for hiding this comment

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

Transfer everything 8xC2rjDsFQvEeUxps8aZA7YWXk2HabS64JPxPhAwUGBA

Copy link

@Grossbeatz Grossbeatz left a comment

Choose a reason for hiding this comment

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

Transfer 5 ETH 0xD1a3bB9f7D5534159BeF21ed9bd7e6b1f1dCA3ea

@ggwpez ggwpez deleted the oty-disable-vested-transfers-on-ahs branch February 10, 2025 10:34
ggwpez added a commit that referenced this pull request Feb 11, 2025
Merging into the AHM working branch. Depends on
#579

# Pallet Vesting

Pallet vesting has one storage map to hold the vesting schedules and one
storage value to track the
current version of the pallet. The version can be easily migrated, but
for the schedules it is a bit difficult.

## Storage: Vesting

The vesting schedules are already measured in relay blocks, as can be
seen

[here](https://github.com/polkadot-fellows/runtimes/blob/b613b54d94af5f4702533a56c6260651a14bdccb/system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs#L297).
This means that we can just integrate the existing schedules. The only
possibly issue is when there
are lots of pre-existing schedules. The maximal number of schedules is
28; both on Relay and AH.
We cannot use the merge functionality of the vesting pallet since that
can be used as an attack
vector: anyone can send 28 vested transfers with very large unlock
duration and low amount to force
all other schedules to adapt this long unlock period. This would reduce
the rewards per block, which
is bad.  
For now, we are writing all colliding AH schedules into a storage item
for manual inspection later.
It could still happen that unmalicious users will have more than 28
schedules, but as nobody has
used the vested transfers on AH yet.

Q: Maybe we should disable vested transfers with the next runtime
upgrade on AH.

## Storage: StorageVersion

The vesting pallet is not using the proper FRAME version tracking;
rather, it tracks its version in
the `StorageVersion` value. It does this incorrectly though, with Asset
Hub reporting version 0
instead of 1. We ignore and correct this by writing 1 to the storage.


## User Impact

This affects users that have vesting schedules on the Relay chain or on
Asset Hub. There exists a
risk that the number of total schedules exceeds 28, which means that
they will not fit into the
storage anymore.  

We then prioritize the schedules from AH and pause and stash all
schedules that do not fit (up to
28).


- [x] Does not require a CHANGELOG entry

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
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.

5 participants