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

Generic transact #28

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

olanod
Copy link

@olanod olanod commented Mar 20, 2022

I figured this is a better place to continue the discussion started in paritytech/polkadot-sdk#853
The idea is decouple Transact to be more generic since the current implementation of XCM expects the encoded call to be runtime specific, the call data is a nested enum that encodes the pallet and extrinsic index which changes from one chain to another, with #22 one can query the pallet information of the remote system to avoid hard coupling among chains but to me it feels more like a not ideal workaround that introduces the extra step of first querying the chain plus the fact that pallet info is very Substrate specific, ideally we should care more about the what to execute and not its location in the other system. Does it make sense?

So for this I propose the "interface identifier" which represents the what to execute regardless of where it's defined. For convenience it doesn't have to be a community maintained registry of calls but can be deterministically and statically generated by creating a hash table that maps generated interface identifiers to the appropriate extrinsic. In the linked issue I give a better example in case it's not clear.

Happy to know what people think of the approach :)

Using a well known identifiers to dispatch the calls instead of the relying on the pallet and extrinsic index
Copy link
Contributor

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

Firstly, I would avoid casually altering pre-existing instructions, at least not without a clear reference implementation and strategy for migration/compatibility.

Secondly, the alterations are essentially useless as they are. Transact (as far as the XCM format goes, anyway) actually is already generic; it involves passing a binary blob and the destination is free to interpret it as it will. All the changes here actually amount to is an additional 33 (32 + 1) opaque bytes to the pre-existing opaque binary blob of arbitrary size. For it to be anything beyond forcing a 33-byte minimum to the opaque data, the meaning of these 33 bytes needs to be well-defined (read: implementable, with a reference implementation).

While #22 is not an ideal solution (I doubt one really exists in such a changing world), the meaning of the extra data is at least well-defined, there is a reference implementation and the scope of the problem it is solving is bounded and well-understood.

@olanod
Copy link
Author

olanod commented May 5, 2022

Thanks for the feedback, I totally understand just throwing in a change like this without a reference implementation is not very useful, but before embarking in such endeavor it's better to first get feedback about whether something like this even makes sense and if it could be a valuable addition.

About the specifics of the instruction, it makes a lot of sense having it as a separate new instruction and not alter the existing one(TransactInterface?). It's true that Transact is generic and the call is an opaque blob, the current implementation could be changed to encode the interface identifier but it's worth exposing it at the instruction level and make the intention of the user more explicit. About the extra size I used [u8; 32] because it's a common size for fast hashes but I think it's safe to halve it and still allow for the interface identifier be a hash that is super unlikely to generate a collision for this use case. About the disambiguation index I'm having second thoughts and could also be removed since it requires previous knowledge of the destination chain which is something I wanted to avoid in the first place, It should be ok to assume the receiver will choose the most appropriate extrinsic in case more than one has the same interface.

Happy to hear more thoughts :)

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