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

[RFC] Remove SetFeesMode instruction #57

Conversation

franciscoaguirre
Copy link
Contributor

The SetFeesMode instruction and the fees_mode register allow for the existence of JIT withdrawal.
This mode complicates the fee mechanism and leads to bugs.
The proposal is to remove said functionality.
This RFC doesn't require RFC#58 but it works alongside it to simplify fee handling.

@xlc
Copy link
Contributor

xlc commented May 23, 2024

Great. For compatibility reason, maybe we should propose a noop instruction to avoid breaking change? Or it doesn't matter because it is not used?

@franciscoaguirre
Copy link
Contributor Author

I don't think it's being used that much, but if it is, this should anyway go in a new version.
The previous version won't be broken, and when converting from the previous version to the new version, this would be a noop.

@franciscoaguirre franciscoaguirre mentioned this pull request May 24, 2024
@acatangiu
Copy link
Contributor

XCM programs logic will change, so I would not remove it or make it a noop because existing programs in the ecosystem will break.

We should deprecate it in v5 (but still respect its contract), then remove it in v6.
(Same for BuyExecution once we have #58)

@franciscoaguirre
Copy link
Contributor Author

XCM programs logic will change, so I would not remove it or make it a noop because existing programs in the ecosystem will break.

We should deprecate it in v5 (but still respect its contract), then remove it in v6. (Same for BuyExecution once we have #58)

This is true but is very unfortunate. Since we have different versions, we should be able to break stuff in a new version. The reason why we can't is because we always convert incoming messages to the latest version. We should instead handle them separately.

If we receive an XCMv3, we should send it over to an executor that uses that version. That way versions actually give us flexibility to break stuff.

@acatangiu
Copy link
Contributor

We should NOT break XCM programs running on supported versions.

"breaking" changes need to come with backwards compatible migration to the new version, or if not possible (like this case), go through deprecation + remove mechanism.


If we receive an XCMv3, we should send it over to an executor that uses that version. That way versions actually give us flexibility to break stuff.

This doesn't scale and adds a lot of complexity and attack surface (mixing multiple executors at once).

For removing functionality the golden standard is to deprecate it, allow time to move away from it to replacements, then remove it.

@franciscoaguirre
Copy link
Contributor Author

When I think of XCM versions, having an executor for each version is the simplest way to know that a supported version will never be broken. Otherwise we are just converting that version to the latest version, and we deprecate between versions, which is weird to see. We should deprecate versions, not have deprecated instructions in multiple versions.

The attack surface only increases because of patches to executors that don't affect the XCM ABI.

Without multiple executors, we restrict what we can do in new versions, complicate the code, and increase the risk of old supported versions failing. Since the older versions jump through more hoops before being executed. We risk having bugs in there. We increase the bug surface.

@acatangiu
Copy link
Contributor

I agree having multiple versioned executors is the "cleanest", but not sure if it's tractable.

This needs more thought, and is worth its own separate discussion/issue/thread IMO.

Regardless of how we implement it, the conclusion on SetFeesMode and BuyExecution is that we need a deprecation story for them to not break existing programs.

@franciscoaguirre
Copy link
Contributor Author

I agree having multiple versioned executors is the "cleanest", but not sure if it's tractable.

This needs more thought, and is worth its own separate discussion/issue/thread IMO.

Regardless of how we implement it, the conclusion on SetFeesMode and BuyExecution is that we need a deprecation story for them to not break existing programs.

Agreed, for now I'll follow the deprecation route, but I'm heavily leaning towards having multiple executors in the future.

@acatangiu
Copy link
Contributor

acatangiu commented Jun 19, 2024

having multiple executors (for different xcm versions) in the future

I was thinking more about this and after zooming out I realised it’s not really possible. Since translating v5<>v4 would not be possible, we wouldn’t just need different executors, but different “pallet-XYZ” versions for pallets sending XCM messages around (pallet-xcm, xtokens, treasury, coretime, identity, etc). Furthermore, the message originator would have to know the versions supported by every hop that message goes through - which is yet another complex on-chain system to build.

So, def not realistic. Looks like instead we need be able to upgrade/downgrade v4<>v5.

@franciscoaguirre
Copy link
Contributor Author

Closing in favor of polkadot-fellows/RFCs#106

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.

3 participants