Add remote proxy support to AH Kusama#535
Conversation
xlc
left a comment
xlc
left a comment
There was a problem hiding this comment.
this is not in polkadot-sdk repo so how do we want to test it on westend?
I will also port it there. I wanted to include it in the next release if possible. People are having more and more these problems of proxies not being on AH and similar. This pallet could solve the issue without requiring a root referendum (we already had one and another is prepared AFAIK). |
Co-authored-by: Xiliang Chen <xlchen1291@gmail.com>
bkchr
changed the title
|
I'd be more comfortable if we also did a security audit for this before adding it to live Polkadot AssetHub |
The scope of the changes is quite narrow. Doing some proper review should be fine. The only difference to the real |
bkchr
changed the title
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Failed to update PR ❌There was an error while trying to keep this PR You may have conflicts More info in the logs 📋 |
|
Fuck why was this merged 🙈 |
|
Apparently some checks were not part of the required checks 🙈 |
|
|
||
| impl< | ||
| LocalProxyType, | ||
| ProxyDefinitionConverter: Convert< |
There was a problem hiding this comment.
There is also a TryConvert for fallible conversions
| type RuntimeCall = RuntimeCall; | ||
| type Currency = Balances; | ||
| type ProxyType = ProxyType; | ||
| type ProxyType = TransparentProxyType<ProxyType>; |
There was a problem hiding this comment.
This is probably interesting for downstream proxy dApps. Going to ping this to some people.
| /// the older the proof can be. This is not only seen as a maximum number, but also as the | ||
| /// maximum difference between the latest and the oldest storage root stored. This means | ||
| /// that if the chain for example did not progress for `MaxStorageRootsToKeep` blocks, only | ||
| /// the latest added storage root will be available for validating proofs. |
There was a problem hiding this comment.
It also means that this is the maximum time after which removal of a proxy will be visible here, or? So users will need to be aware that there is a delay of removing a proxy now.
| pub struct Pallet<T, I = ()>(_); | ||
|
|
||
| /// Stores the last [`Config::MaxStorageRootsToKeep`] block to storage root mappings of the | ||
| /// target chain. |
There was a problem hiding this comment.
One comment please on the order of entries here.
|
|
||
| // We always remove all the old items before, thus there should always be space in | ||
| // the vector. | ||
| let _res = roots.try_push((block, hash)); |
There was a problem hiding this comment.
Could use defensive_proof here to get a production error log.
| /// chain. | ||
| /// | ||
| /// The dispatch origin for this call must be _Signed_. The difference to | ||
| /// [`Self::remote_proxy`] is that the proof nees to registered before using |
| proxy_definitions | ||
| .into_iter() | ||
| .filter_map(T::RemoteProxy::remote_to_local_proxy_defintion) | ||
| .find(f) |
There was a problem hiding this comment.
I guess in the case that there are multiple proxies and no force type, it will just pick the first one?
Should not produce issues i think, just an observation.
9 participants
This adds remote proxy support to AssetHub on Polkadot and Kusama. Currently it is configured only to support using proxies registered on the relay chain to work on AssetHub. In the future this can be expanded.