Fix ordering of message ids if the trait is implemented before the inherent section #1235
Conversation
inherent_ids traits_ids Now the ordering is not changed. It is important for dispatching logic and wildcard. Dispatching takes the global index of the method but if ordering is changed, it will use wrong id. In the case of wildcard it breaks it at all if trait was implemented before inherent section.
xgreenx
requested review from
Robbepop,
cmichi,
ascjones and
HCastano
as code owners
There was a problem hiding this comment.
I see you have the example, but could you also add a ui test for this case too, e.g. /~https://github.com/paritytech/ink/blob/e8d4739649293a458c3958bec802d2d750067d98/crates/lang/tests/ui/contract/pass/message-selector.rs
ascjones
changed the title
|
CI can't pull my branch=( |
|
At a glance looks like it is not configured to pull from forks. Maybe we need to fix that. Anyway we can go ahead and merge if we get another approval since the waterfall is not a requirement for merge. @cmichi |
| @@ -34,7 +59,7 @@ fn main() { | |||
| >>::IDS[0] | |||
| }, | |||
| >>::SELECTOR, | |||
| [0x5A, 0x6A, 0xC1, 0x5D], | |||
| [0xFB, 0xAB, 0x03, 0xCE], | |||
There was a problem hiding this comment.
So this is a breaking change. Will require a minor release.
| @@ -33,6 +33,30 @@ pub mod proxy { | |||
| admin: AccountId, | |||
| } | |||
|
|
|||
| /// The trait provides information about `Proxy` | |||
There was a problem hiding this comment.
What is the reason why you added this trait btw.?
There was a problem hiding this comment.
I added it to test the behavior of the wildcard in case of the trait is implemented before the inherent section.
But it was before I added the UI test=) So now it is not so actual but we can keep it to verify that the behavior is right during the e2e test.
Co-authored-by: Michael Müller <mich@elmueller.net>
| /// The trait provides information about the forward pattern used in this contract. | ||
| #[ink_lang::trait_definition] | ||
| pub trait Forwarder { | ||
| /// Returns the `AccountId` of the contract that calls are forward to. | ||
| #[ink(message)] | ||
| fn forward_to(&self) -> AccountId; | ||
|
|
||
| /// Returns the `AccountId` of the admin of the current contract. | ||
| #[ink(message)] | ||
| fn admin(&self) -> AccountId; | ||
| } | ||
|
|
||
| impl Forwarder for Proxy { | ||
| #[ink(message)] | ||
| fn forward_to(&self) -> AccountId { | ||
| self.forward_to | ||
| } | ||
|
|
||
| #[ink(message)] | ||
| fn admin(&self) -> AccountId { | ||
| self.admin | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
| /// The trait provides information about the forward pattern used in this contract. | |
| #[ink_lang::trait_definition] | |
| pub trait Forwarder { | |
| /// Returns the `AccountId` of the contract that calls are forward to. | |
| #[ink(message)] | |
| fn forward_to(&self) -> AccountId; | |
| /// Returns the `AccountId` of the admin of the current contract. | |
| #[ink(message)] | |
| fn admin(&self) -> AccountId; | |
| } | |
| impl Forwarder for Proxy { | |
| #[ink(message)] | |
| fn forward_to(&self) -> AccountId { | |
| self.forward_to | |
| } | |
| #[ink(message)] | |
| fn admin(&self) -> AccountId { | |
| self.admin | |
| } | |
| } |
I would remove it, as it doesn't serve a function to the example. I'd like to keep the examples as minimal as possible to focus on illustrating the point (in this case how to build an upgradeable contract).
PR looks good besides that!
Fixed the ordering of ids. Before it was sorted:
inherent_ids
traits_ids
Now the ordering is not changed. It is important for dispatching logic and wildcard. Dispatching takes the global index of the method but if the order is changed, it will use the wrong id. In the case of a wildcard, it breaks it at all if the trait was implemented before the inherent section.