Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 8 commits
db3822d
9f93a19
0cf749e
cfba268
7d404f4
1f9e6f5
985365f
5db72af
cc946da
54fd609
013ea59
a028c55
3e0d8d1
ada84b3
0029e18
f000f24
0124e14
fcb684d
64feb63
df9956c
f1a1b9d
1cda4b9
246570d
f88fcf7
f67a88a
27d6b46
0e039e7
3b48940
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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.https://github.com/peetzweg/pjs-api/blob/pz/ink-v5/packages/api-contract/src/Abi/index.ts#L165-L183
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.
I am getting myself familiar with the correlation between Ink v5 and the conversion process above, and found this on the following
signatureTopic
: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.