Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[pallet-proxy] Making (height, ext_index) optional in kill_pure() #12411

Closed
wants to merge 3 commits into from

Conversation

arturgontijo
Copy link
Contributor

This PR removes the need to set height and ext_index parameters in the kill_pure() extrinsic making it more user friendly.

I've put both parameters in an Option<(T::BlockNumber, u32)> to keep it back compatible with the current design.

If this PR lands, the spawner (only) will be able to kill a pure proxy by sending its proxy_type and index (avoiding keep track of when it was created).

Copy link
Contributor

@xlc xlc left a comment

Choose a reason for hiding this comment

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

This is a breaking change for no good reason.
The whole point of pure proxy is that the ownership can change and requiting the spawner account to kill the proxy is just a bad idea.

@arturgontijo
Copy link
Contributor Author

Thanks for the review @xlc.

Just pushed back the signer (pure proxy account) logic to the kill_pure() extrinsic.
Do you think that it is still a breaking change doing that way?

My main point here is that index is something that (we already use as parameter) user controls and, imo, is more easy to keep track (the height+ext_index) when opting to kill a pure proxy.

@kianenigma
Copy link
Contributor

Do you think that it is still a breaking change doing that way?

anything changing the transaction encoding is breaking. You would still need to append an extra 0x0 to the call to encode the new format.

That being said, I don't have a strong opinion about whether this is needed or not.

Something that I would consider is adding more transactions instead of removing existing ones. I am playing around with this idea in e.g. #12001. You can make a new kill_pure_without_height or something like that. This is just an idea.

@arturgontijo
Copy link
Contributor Author

Hey @kianenigma, thanks for the heads up!
Just pushed the new extrinsic but TBH I didn't like the final result (too over, mayber) =/

So I'm closing this for now, as I'm the only one that found the height+ext_index kind of annoying.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants