feat: ink! v5

[peetzweg]

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.

[cmichi]

@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 polkadot-js/api.

[TarikGul]

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 toV5.

[peetzweg]

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 toV5.

@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. As example the signature_topic of this Transfer event would be the result of

blake2b256("Transfer(Option<AccountId>,Option<AccountId>,Balance)")

    #[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 toV5 as the payload of the EventRecord is different.

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 topics are ignored for decoding.

deployed v4 contract call emits event like this => (data: 0x01_abc..., topics: [?]) 
// topics empty? At least ignored for matching event with metadata, 
// but first byte of `data` is `1` the index of the event in the metadata v4 events array, 
// payload of event is `0xabc`

Important to note here, is everything compiled and deployed with ink4 and prior will still work this way.

In v5 however the topic of an EventRecord contains the newly added signature_topic topic value for matching. And the data of a event is not prefix with the event index anymore.

deployed v5 contract call emits event like this => (data: 0xabc, topic: [0xsignature_topic]) 
// => match event by signature_data in v5 metadata, 
// payload of event is `0xabc`

So as v4 and v5 emit completely different data in the raw event, I think it might be best to make the actual Abi instance aware the metadata version used to instantiate it instead of having one Abi for all versions, deciding which path to take by checking which values are present or not. This is how I done it in this pr for now.

Not sure how much work this rework might imply. 🤔

[TarikGul]

So as v4 and v5 emit completely different data in the raw event, I think it might be best to make the actual Abi instance aware the metadata version used to instantiate it instead of having one Abi for all versions, deciding which path to take by checking which values are present or not.

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 signature_topic, and module_path to undefined and or if we set them to an Option).

Have we tried to run the current solution against a v4 contract to see the sideaffects?

[TarikGul]

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 toLatest which would be v5 the general structure would still be valid for the v4 structure?

[peetzweg]

@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.

[TarikGul]

@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:

1. We do some trial by fire testing with a "test" contract that is using v4 with your current implementation, and we test to see how decoding reacts via v5 conversion. Not an ideal way to go about things but an honest way to see effects.

2. Build on your suggestion of reworking the ABI.

3. Build on your suggestion, but only focus on the getLatestMetadata call. ContractMetadataLatest could resolve to 2 types: ContractMetadataV4 | ContractMetadataV5. So everything v4 and before will resolve to v4 anything v5 and above will resolve to v5. There is a few big issues with this:

• Maintenance: when a new contracts version causes another breaking change it would be a real pain.
• This might still involve many changes in the ABI. So basically it's another type of rework of the ABI suggestion you had.

[peetzweg]

Okay briefly questioned my sanity but I've added two new test for v4 and v5 decoding event payload. inkv5 compiled metadata still contained "version": "4", which caused it to migrate from v4 to v5 and setting the new fields to undefined.

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. Jaco already added a comment about the decodeEvent(data: Bytes) method being unstable. This might become a breaking change, as we don't know who is using it directly. However, I think this method should just receive the full EventRecord instead of the destructured event.data.

For v5 we need the topic of the EventRecord. External users might not know what to pass as the signature_topic into the decodeEvent(data:Bytes, signatureTopic:Hash) method, so decodeEvent(event:EventRecord) would be more future proof and easier to understand.

[TarikGul]

I think it would be best to work with concrete metadata version in the Abi Class and narrow implementations based on that version type.

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 :)

Jaco already added a comment about the decodeEvent(data: Bytes) method being unstable. This might become a breaking change, as we don't know who is using it directly. However, I think this method should just receive the full EventRecord instead of the destructured event.data.

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.

[peetzweg]

@TarikGul changed the method signature as discussed above in this commit: https://github.com/polkadot-js/api/pull/5791/commits/013ea59f70b265c7038936dbf8fa85729559a4a2

Furthermore, I drafted a version of the Abi which is aware of it's version in this commit: https://github.com/polkadot-js/api/pull/5791/commits/a028c554ee4260af08084c1c4818053a076e0577

Would appreciate a review of it. I'm not yet super happy with it, got a bit ugly with externalizing the version field to the Abi class. Maybe this could be done a bit better if we decide to move a version field into the ContractMetadataVX, however seems maybe we can leverage this ContractMetadata enum?

As of now I'm somewhat fine with the implementation. I haven't removed any of the previously used code yet, first and foremost toLatest => toLatestCompatible.

[TarikGul]

@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 version field to the Abi class. Maybe this could be done a bit better if we decide to move a version field into the ContractMetadataVX, however seems maybe we can leverage this ContractMetadata enum?

As of now I'm somewhat fine with the implementation. I haven't removed any of the previously used code yet, first and foremost toLatest => toLatestCompatible.

So I just had a look, did a moreso soft review, but It looks really good to me and feels like its going in the right direction.

[peetzweg]

@TarikGul Okay I think this is ready for a proper review. For me this is the way to go for now. 🙌

Once this is merged I will make sure the metadata in here is up to date and maybe adapt minor stuff as the "version" with this pr no longer Text but a u64.

https://github.com/paritytech/ink/pull/2126

[TarikGul]

@TarikGul Okay I think this is ready for a proper review. For me this is the way to go for now. 🙌

Once this is merged I will make sure the metadata in here is up to date and maybe adapt minor stuff as the "version" with this pr no longer Text but a u64.

paritytech/ink#2126

Super exciting 🚀 Nice I will do a full review by the EOD today :) thanks again for tackling this!

[peetzweg]

@TarikGul tackled all requested changes. ~~However, I would wait for a new inkv5.rc2 candidate to make sure the contract metadata has the expected shape as it was changed again here: https://github.com/paritytech/ink/pull/2126.~~

~~I manually patched the included metadata here https://github.com/polkadot-js/api/pull/5791/commits/f67a88af4d80dccb3566c683210836592b0bd98b, but just to be on the save side. ✌️~~

Just added updated the contract metadata with the most recent master branch of ink!, https://github.com/polkadot-js/api/pull/5791/commits/27d6b46afaded8f0aabc2e570afc38664abd0f03.

So ready when you are. 🚀

[TarikGul]

@TarikGul tackled all requested changes. ~However, I would wait for a new inkv5.rc2 candidate to make sure the contract metadata has the expected shape as it was changed again here: paritytech/ink#2126.~

~I manually patched the included metadata here f67a88a, but just to be on the save side. ✌️~

Just added updated the contract metadata with the most recent master branch of ink!, 27d6b46.

So ready when you are. 🚀

Just a quick question given 27d6b46, was it suppose to be set to 5.0.0-rc2?

[peetzweg]

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 refresh the contract metadata with output from the release version of ink!v5.

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",
...
}

[cmichi]

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 polkadot-js/api. Then we can say that ink! 5 is compatible with the latest polkadot-js/api release, which would be a big thing.

[TarikGul]

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 refresh the contract metadata with output from the release version of ink!v5.

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",
...
}

Ahh okay thanks for the clear explanation, this should be fine!

[TarikGul]

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 polkadot-js/api. Then we can say that ink! 5 is compatible with the latest polkadot-js/api release, which would be a big thing.

@cmichi Would it be okay if we did the polkadot-js/api release on Monday to keep in cadence with the release schedule/process. Today, the extension, ui, tools, and phishing will be released, and the next day apps.

[cmichi]

@cmichi Would it be okay if we did the polkadot-js/api release on Monday to keep in cadence with the release schedule/process. Today, the extension, ui, tools, and phishing will be released, and the next day apps.

@TarikGul Yup, that's fine! What will be the version number of that release?

[TarikGul]

@cmichi Would it be okay if we did the polkadot-js/api release on Monday to keep in cadence with the release schedule/process. Today, the extension, ui, tools, and phishing will be released, and the next day apps.

@TarikGul Yup, that's fine! What will be the version number of that release?

Thanks so much! Version will be - 10.12.0 to reflect a feature.

[TarikGul]

@peetzweg Good for me to merge it in?

[peetzweg]

@ascjones

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 Transfer event ink!v5 and an anonymous version of ink!v5. Both contain several hashes in the topic field. As mentioned the anonymous ink!v5 version dismisses the first element, signature_topic.

However, just with the contract metadata it's impossible to know if the first hash of the topics is an actual signature_topic or a hash of a field of the event (#[ink(topic)]).

Yes, the anonymous event metadata is still included with signature_topic: null, so we could just try all of the anonymous then. But if two events have the same payload types we wouldn't know which one it is.*

Should we than just omit the Label of the event and use "Anonymous", log a warning or call it MaybeTransfer? 😅

~~The usecase for #[ink(topic)] without any information about it in the metadata is a bit unclear to me as well. 🤷~~ (Ah, that's the "indexed": true part)

*An that is already the case for Transfer and Approval in the erc20 contract. (Yes transfer has this Option<AccountId> and Approval just AccountId but why is there an Option in a Transfer event in the first place?)

    #[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",
      ],
    },
  },
];

[ascjones]

Should we than just omit the Label of the event and use "Anonymous", log a warning or call it MaybeTransfer

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.

[peetzweg]

@TarikGul @ascjones in this commit https://github.com/polkadot-js/api/pull/5791/commits/0e039e795131214aeb9cd0ebcb50ab70e72e608f 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 [#[ink(topic)]](https://use.ink/macros-attributes/topic) was used.

FYI in the referenced erc20 example contract we have two events, Transfer and Approval. I made the "Transfer" anonymous.

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 UndecodedEvent with the plain bytes of the event is possible but takes a lot of refactoring the types up the hierarchy. I'm not sure if I can deliver it in time for the planned release on monday.

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?

[ascjones]

The other options of returning an UndecodedEvent with the plain bytes of the event is possible but takes a lot of refactoring the types up the hierarchy. I'm not sure if I can deliver it in time for the planned release on monday.

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)

[TarikGul]

Once this is in by EOD, i will most likely do the release Sunday evening in order to stay ahead of CET!

[TarikGul]

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 0 as the patch version. It always needs to end in > 0...

[polkadot-js-bot]

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.