-
Notifications
You must be signed in to change notification settings - Fork 352
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
feat: ink! v5 #5791
feat: ink! v5 #5791
Conversation
module_path: undefined, | ||
signature_topic: undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do we consider this a breaking change and don't allow converting from v4 to v5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the implications of this convert process, but here's how we describe the change:
Previously the order of the events in the
events
array was significant (i.e. the first one had an implied index of0
), and this index could be used to determine which event to decode. Now that is replaced by thesignature_topic
, and the order of the events in the metadata no longer has any significance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual decoding takes place here, using the first topic of the substrate event treating as the signature_topic
. Still need to add a proper test for it.
As all previous version where migrate-able the underlying code always works converts it to the latest version of the metadata. However, this time there is newly added data we can't create for older version of the metadata. This would imply that pjs needs switch decoding events based on the version of the metadata and not handle everything the same way. I think this would be nice to get input from @jacogr how to change the codebase to handle this. How I have done it is not the prettiest. Potentially could come up with a draft though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, this time there is newly added data we can't create for older version of the metadata.
I am getting myself familiar with the correlation between Ink v5 and the conversion process above, and found this on the following signatureTopic
:
/// # The Signature Topic
///
/// By default, the [`ink::Event::SIGNATURE_TOPIC`] is calculated as follows:
///
/// `blake2b("EventStructName(field1_type_name,field2_type_name)")`
/// The hashing of the topic is done at codegen time in the derive macro, and as such only has
/// access to the **names** of the field types as they appear in the code. As such, if the
/// name of a field of a struct changes, the signature topic will change too, even if the
/// concrete type itself has not changed. This can happen with type aliases, generics, or a
/// change in the use of a `path::to::Type` qualification.
-
Would it be invalid to construct the signatureTopic for a v4 event when converting to v5?
-
In this case do we not have the valid info/data to construct it if the conversion can be considered valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if relevant here, but the topic calculation changed in general from v4 to v5, so it's a breaking change for any clients that used topics to filter for events.
We have a migration guide for the events changes here.
@jacogr Could you take a look at this PR? That would be very helpful for us, as we would like to release ink! 5 with a compatible |
Definitely think we should get this in as soon as we can, I'll be looking into the metadata conversion to see some viable solutions: Specifically for |
@TarikGul, I just asked @ascjones a bunch of questions regarding the changes from events 1.0 to events 2.0. Although it's possible to derive a v5 version of the metadata. As it's just the hash of the Event label and it's types.
#[ink(event)]
pub struct Transfer {
#[ink(topic)]
from: Option<AccountId>,
#[ink(topic)]
to: Option<AccountId>,
value: Balance,
} All of this data would be available in v4 metadata. However it would not make too much sense to migrate v4 metadata In v4 the first byte of the event data payload is the index of the event type in the event list of the metadata. Potentially emitted
Important to note here, is everything compiled and deployed with ink4 and prior will still work this way. In v5 however the
So as v4 and v5 emit completely different data in the raw event, I think it might be best to make the actual Not sure how much work this rework might imply. 🤔 |
This is definitely a valid solution. I think the common concern here is this would introduce a large change in the Abi. In terms of work I think it would be a large amount. That being said it is probably one of the best options. I'm trying wrap my head around the current solution to find some viability (setting Have we tried to run the current solution against a v4 contract to see the sideaffects? |
As far as I can tell the conversion is there to allow the Abi to work with a single version of the metadata. Correct me if I am wrong but if a v4 contract is being used and the abi transforms the metadata |
@TarikGul, so the conversion from v4 to v5 is possible. However, I'm not sure if the metadata ink v5 spits out still adheres to the order of events as it was in v4. The order was important to decode the event in v4. And as the Abi class right now always just works with the "latest" of the metadata, it's a bit ugly to determine if an event should be decoded in v4 style or v5 style. |
I see mainly a few cases here:
|
Okay briefly questioned my sanity but I've added two new test for v4 and v5 decoding event payload. inkv5 compiled metadata still contained Understanding this overall flow know better than before. As we already have an "ugly" v4 workaround and we would add another for v5, I think it would be best to work with concrete metadata version in the Abi Class and narrow implementations based on that version type. For v5 we need the |
Considering your findings I agree. Better to take care of this now, and do it correctly than have a hacky less than ideal situation. It will also help decouple each version implementation in a way where it'll be easier to debug and create new versions in the future. Im on board, love it :)
Good point. For now I guess we can aim to just do a MINOR bump for this feature, and note the internal changes in the PR description. |
…Event(record:EventRecord)` which includes the event and the topic for decoding.
@TarikGul changed the method signature as discussed above in this commit: 013ea59 Furthermore, I drafted a version of the Abi which is aware of it's version in this commit: a028c55 Would appreciate a review of it. I'm not yet super happy with it, got a bit ugly with externalizing the As of now I'm somewhat fine with the implementation. I haven't removed any of the previously used code yet, first and foremost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, excellent job! Looks really solid. Just a few nits.
Some thoughts on the decision for the specific versioning in the ABI. I think moving forward its probably the best choice, and allows for cleaner abstraction of logic. Things were a bit coupled earlier which caused our main issue of metadata conversion. Regardless, if there is some type of breaking change in the future it will always be difficult, but now there is a pretty solid track for multiple version support.
All that being said, very pleased with how it turned out! Nice job! When those few nits are resolved, I think we can give this a ✅ .
@TarikGul tackled all requested changes.
Just added updated the contract metadata with the most recent So ready when you are. 🚀 |
Just a quick question given 27d6b46, was it suppose to be set to 5.0.0-rc2? |
It's bit tricky right now. There is no ink! 5.rc2 out just yet. So the contract metadata included in this PR are from these development version of ink!v5 aka master branch. The proper way would be probably to wait for the release of ink!v5 and then one final time How imminent do you think the release of ink!v5 is, @cmichi? Afaik the metadata is not about to change anymore for v5, so the only difference will be in the language field. But I'm fine also waiting for the actual release. 🤷 {
...
"language": "ink! 5.0.0-rc.1",
"language": "ink! 5.0.0",
...
} |
We'll release either end of this week or first thing next week. So you can already update to "ink! 5.0.0" and merge with that. There'll be no more metadata changes. @TarikGul It would be great if following the merge of this PR you could then issue a new release of |
Ahh okay thanks for the clear explanation, this should be fine! |
@cmichi Would it be okay if we did the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Thanks so much! Version will be - |
@peetzweg Good for me to merge it in? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I think we need to handle anonymous events though https://use.ink/5.x/basics/events#anonymous-events
That does not feel super straightforward to add to me. 🤔 Here are EventRecords of an erc20 However, just with the contract metadata it's impossible to know if the first hash of the topics is an actual Yes, the Should we than just omit the Label of the event and use
*An that is already the case for #[ink(event)]
#[ink(anonymous)]
pub struct Transfer {
#[ink(topic)]
from: Option<AccountId>,
#[ink(topic)]
to: Option<AccountId>,
value: Balance,
} [
{
EventRecordInkV5Event: {
phase: {
applyExtrinsic: 1,
},
event: {
index: "0x0803",
data: [
"5Gzf9pZsGj3SztG48Qxo7gdgCarAvuQfyyZXcGr8Nd7jA9Pm",
"0x01d43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d01d43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d00f0e4888f4463010000000000000000",
],
},
topics: [
"0xb5b61a3e6a21a16be4f044b517c28ac692492f73c5bfd3f60178ad98c767f4cb",
"0xd43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d",
"0x8eaf04151687736326c9fea17e25fc5287613693c912909cb226aa4794f26a48",
],
},
},
{
EventRecordInkV5AnonymousEvent: {
phase: {
applyExtrinsic: 1,
},
event: {
index: "0x0803",
data: [
"5DxG9VD3UepN5GDkL5Jqyc8dum7NACfu7aFPAqYBuSNw6hq7",
"0x01d43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d018eaf04151687736326c9fea17e25fc5287613693c912909cb226aa4794f26a4800505a4f7e9f4eb10600000000000000",
],
},
topics: [
"0xd43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d",
"0x8eaf04151687736326c9fea17e25fc5287613693c912909cb226aa4794f26a48",
],
},
},
]; |
To be safe for the initial version we can just say that if we can't find a matching event for the first topic and there exists an anonymous event in the metadata then we just label it as "Anonymous" and provide the raw bytes. |
@TarikGul @ascjones in this commit 0e039e7 I've implemented handling and trying to determine anonymous events. If only a single anonymous event exists in the contract, than this works just fine. When we have two or more anonymous events it tries to distinguish them by the amount of indexed parameters. Which could help separating two or more anonymous events if FYI in the referenced erc20 example contract we have two events, Afaik trying to best effort decoding the for all the anonymous events does not work with the current implementation of pjs, as it does not fail if the data is to long, so it could lead to false positive cases. The other options of returning an Furthermore this PR is quite big already and should handle all cases gracefully and will only throw if the contract is using multiple anonymous events. I suggest to add it in another PR, wdyt? |
return potentialEvents[0].fromU8a(data); | ||
} | ||
|
||
throw new Error('Unable to determine event'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other case to consider is if a cross contract call occurs, which in turn raises an event. Since the event is raised from another contract we don't have the metadata here...so we might not want to raise an error and just give back the raw bytes instead of attempting to decode?
This scenario might also lead to a false positive in the heuristic for above for determining anon events, if the foreign event has the same number of topics 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, so if contract A calls a function on B which emits an event, the event will be emitted by A and not B?
Feels like it makes sense as users might not know anything about B so can only listen to A.
As of now I have to little knowledge on what's happening above this little 'Abi.decodeEvent world' to have an idea how to handle this the best, yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, so if contract A calls a function on B which emits an event, the event will be emitted by A and not B?
It depends, using forward_call
(contract A calling contract B), the account id topic will be for contract B so would be easy to distinguish
The other case is using delegate_call
where the code of contract B is invoked from contract A, in this case the account id topic would be of contract A.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay! Will tackle this in a follow up feature pr! 🚀
In that case we can do it in a follow up, better to have some support for the new event format - with the caveat that it will error if it encounters an unknown event (raised from another contract) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
More work required to figure out the best way to handle anon and foreign events, but this is a great start.
Once this is in by EOD, i will most likely do the release Sunday evening in order to stay ahead of CET! |
The release should be out in 10.12.1 in a few moments. To my surprise I tried to do a release last night after checking everything and woke up to the CI failing. Ends up you can't do a release in PJS with a |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Trying to make your life, @jacogr, as easy as possible to resolve #5789. I left a few comments about parts I'm not sure what is the correct way to do here and would appreciate your input.
Will look into writing a test, making sure both decoding methods work as intended for v4 & v5 contracts.