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

Add find_last for events_types #821

Merged
merged 1 commit into from
Feb 13, 2023
Merged

Add find_last for events_types #821

merged 1 commit into from
Feb 13, 2023

Conversation

lean-apple
Copy link
Contributor

In order to optimise the code present in this PR, used for this issue fixing the wrong return of contract account id with cargo contract instantiate

At the moment, only the method was added, yet more integration tests should be.

As @ascjones suggests, finding 'a pallet where we know that two of the same type of event will be raised by the same extrinsic' should be more relevant than just adding the same integration tests where find_first is used,

What do you think would be the best @jsdw ?

Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

Looks good. Regarding testing, it looks like you can add a relatively simple unit test: see the tests in this file.

@jsdw
Copy link
Collaborator

jsdw commented Feb 13, 2023

There aren't any tests for find or find_first yet either (just the iterating on which they are based). It would be amazing to have a short test for each if you are able, but I know it's more than you would have been bargaining for with this addition so it's up to you :)

@lean-apple
Copy link
Contributor Author

In fact we need this function quickly to fix this issue I will keep in mind to add these tests for the 3 find methods/open an issue for it, can we merge it in the meantime ?

@jsdw
Copy link
Collaborator

jsdw commented Feb 13, 2023

This is a tiny addition so I'm happy to approve for now!

@lean-apple lean-apple merged commit 1092e8b into master Feb 13, 2023
@lean-apple lean-apple deleted the ln-event-add-find-last branch February 13, 2023 11:53
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.

4 participants