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

Fix reserved amounts for pure proxies #237

Open
gavofyork opened this issue Dec 12, 2022 · 3 comments
Open

Fix reserved amounts for pure proxies #237

gavofyork opened this issue Dec 12, 2022 · 3 comments
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@gavofyork
Copy link
Member

Framing

Regular Proxy

slave ---calls---> add_proxy(master)

  • slave must have funds
  • slave has additional amount reserved to pay for recording the relationship.

Pure Proxy

master ---calls---> pure_proxy() which creates slave:

  • master must have funds
  • master has amount reserved to pay for recording the relationship.

Problem and solution

When these two models are mixed (i.e. when a pure proxy is created which then adds a regular proxy), then problems ensue: more is reserved than should be and not enough can be freed.

Instead of having two different places where the funds can be held in reserve (depending on whether the relationship was started with a pure proxy or a regular proxy), it should always be held by the slave. When a pure proxy is created, the funds should not be reserved in the master account, but first transferred into the slave account and reserved there. kill_pure must do the reverse: remove all records/deposits and transfer them back to the master account.

See bkchr-demonstrate-proxy-reserve-issue for one test demonstrating the issue.

@xlc
Copy link
Contributor

xlc commented Dec 12, 2022

related paritytech/substrate#8550

@dandanlen
Copy link

I thought I might illustrate this with a specific motivating use-case where we mix pure/regular proxy semantics at Chainflip:

To implement a vault on polkadot, we're using a pure account with a proxy controlled by threshold multisig. We periodically update the ownership of the pure vault account by removing the old multisig proxy and adding a new one.

Currently we need to fund both the initial multisig proxy account and the pure account; otherwise there aren't enough funds in the pure account to pay for the addition of the next proxy. If the funds were transferred to the pure account on creation, we would be able to remove the old and add a new for net-zero extra funds.

Secondly, the funds reserved in the initial 'spawner' proxy account during creation stay reserved until (maybe one day) the pure account is killed. Given that, in our case, the creator account is a multisig, and that the account was deliberately removed from the proxy list, it's likely that by the time the funds are recovered, the spawning multisig will be defunct.

I agree that transferring the funds to the pure account when it is created would make pure accounts more intuitive to use.

Additionally, I would suggest allowing the caller of kill_pure to specify which account should be the beneficiary of the freed-up base deposit - there's no fundamental reason that the funds need to be returned to spawner, and indeed one of the main motivations for pure accounts is to transfer ownership. So seems reasonable to allow whoever is authorised to kill the account to also decide what to do with the deposit.

FWIW both of the above issues are minor and easy to work around once you're aware of them: make sure the pure account is funded (and learn to live with the fact that you might never see your deposit again!).

@adamsteeber
Copy link

I don't know if the following is an artifact of this issue, but I was closing up an account that I had used for the configuration I prepared in Referendum 145. I used HCRNXiHK6jvADLnXL5nvRpCnE2D2VMu3WA9EriEBDiJJr9T (i.e. the master) to create 9 pure proxies, added the Treasury as an any proxy to each of them, then used HCRNXiHK6jvADLnXL5nvRpCnE2D2VMu3WA9EriEBDiJJr9T to remove itself as a proxy of each of them (probably should have used killPure instead of removeProxy). I understood that I would lose the reserve amount and wrote it off. I was also reimbursed in Referendum 156 so it's not really an issue of getting the funds back.

Today I decided to reap whatever transferrable funds were left in HCRNXiHK6jvADLnXL5nvRpCnE2D2VMu3WA9EriEBDiJJr9T. I used balances.transferAll, expecting it to fail or leave some dust to maintain the reserve amount for the pure proxies. However, Kusama just killed the account and the reserve funds disappeared. I enlivened the account again to see if it would restore the reserve amount but it looks like the reserve balance just vanished.

So now there is no such account with a proper reserved balance recorded in the chain state to reflect the proxy relationships recorded here:

const proxies = [
  "HeFL76wT2rZPQd3j4EnjothdLzLoiYUL3Tqc7kdqsJTznMe",
  "DedtrfJnGapVYdds7F4rM1jPZis19NnJS9aHdjLFmfrmKa5",
  "EcQgA6iB5LXcepGevvJU8MyLxKVs19xNks83xK6FsUzP7h2",
  "GR7Ne8N1QgK36dweG14KcVX5gfhR1ExnAJ7jVhTGvH6qP5z",
  "EeytoLwHVy6xSFN1MtbeKAmMa5J6iM3TuKRgEFaL1ifKe2u",
  "D5ySbJETHTAFpJnoZ8NSLuDSr367G1xioXz9yhm3WDfbeAM",
  "Eibm3W68F8Nev68KBnreUr1Q33SmnwaWFGt37h2zT17LSn3",
  "DGiVvkbo77knCEJLJ54DEE154fyJSY5TVxmAEmJneJioeWb",
  "Gk2wxmpU6R1rmAvD49aGp9Eqo2C9Q69JTxDAexxgVvQKC4h"
];

const query = await api.query.proxy.proxies.multi(proxies);

console.log(query);

I don't know if that matters or if it's intentional, but I assume there should exist some account that holds a reserve balance to reflect the storage of these proxy relationships.

Idk if this report is at all tip worthy but in any case:

Kusama address: GqC37KSFFeGAoL7YxSeP1YDwr85WJvLmDDQiSaprTDAm8Jj

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added T1-FRAME This PR/Issue is related to core FRAME, the framework. and removed T1-runtime labels Aug 25, 2023
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
* auto exchange tx relay dashboard

* cargo fmt --all

* single metrics startup fn
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
* auto exchange tx relay dashboard

* cargo fmt --all

* single metrics startup fn
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* auto exchange tx relay dashboard

* cargo fmt --all

* single metrics startup fn
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* auto exchange tx relay dashboard

* cargo fmt --all

* single metrics startup fn
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* auto exchange tx relay dashboard

* cargo fmt --all

* single metrics startup fn
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* auto exchange tx relay dashboard

* cargo fmt --all

* single metrics startup fn
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* auto exchange tx relay dashboard

* cargo fmt --all

* single metrics startup fn
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* auto exchange tx relay dashboard

* cargo fmt --all

* single metrics startup fn
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
* auto exchange tx relay dashboard

* cargo fmt --all

* single metrics startup fn
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
* auto exchange tx relay dashboard

* cargo fmt --all

* single metrics startup fn
jonathanudd pushed a commit to jonathanudd/polkadot-sdk that referenced this issue Apr 10, 2024
* auto exchange tx relay dashboard

* cargo fmt --all

* single metrics startup fn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: To Do
Development

No branches or pull requests

6 participants