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 indexed args to events #540

Merged
merged 4 commits into from
Sep 29, 2023
Merged

Add indexed args to events #540

merged 4 commits into from
Sep 29, 2023

Conversation

rin-st
Copy link
Member

@rin-st rin-st commented Sep 22, 2023

Description

Started here

Problem:
After migrating to viem and wagmi 1+ access to event parameters changed.

Before (ethers.js):
All the parameters of the event were accessible by index of argument in listener function or basically by index in args array.

After. We have two cases:

  • parameters of event in solidity contract are named, for example: SomeEvent(uint256 arg1, uint256 arg2). We can access that event parameters on frontend only by param name: const {arg1, arg2} = event.args
  • parameters not named, for example: SomeEvent(uint256, uint256). We can access that event parameters on frontend only by index: const [arg1, arg2] = event.args or the same const arg1 = event.args[0]; const arg2 = event.args[1];

so writing frontend we're depending on how contract events are written and can't write universal code

Solution:
Add indexed parameters for frontend for named event parameters too. So for case with named params SomeEvent(uint256 arg1, uint256 arg2) we can access event params by param name OR by index of param.
And it means we can always access event parameters at least by index

Additional Information

Your ENS/address:
0xrinat

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

Tysm @rin-st 🙌, Looking great to me !

Just fixed types for EventSubsriber because now people can also access through the index :

Screenshot 2023-09-26 at 5 18 13 PM

Regarding useScaffoldEvenHistory currently in the main branch too data : any[] we should fix it to other PR

Let's see what others have to say about indexed args 🙌

@rin-st
Copy link
Member Author

rin-st commented Sep 26, 2023

Just fixed types for EventSubsriber

Nice! 👍

Copy link
Member

@damianmarti damianmarti left a comment

Choose a reason for hiding this comment

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

LGTM and it seems a nice improvement. Thanks @rin-st !!

@rin-st
Copy link
Member Author

rin-st commented Sep 29, 2023

@technophile-04 your types doesn't work for other contract, could you check it?
scaffold-eth/se-2-challenges#93 (comment)

upd. It seems it's because of other viem version

@technophile-04
Copy link
Collaborator

Actually Rinat I think we missed updating contract.ts in that PR looking at files changed => https://github.com/scaffold-eth/se-2-challenges/pull/93/files

Also I think we updated the types just for useScaffoldEventSubscriber and I see in stakings.tsx we are using useScaffoldEventHIstory :

I just tried adding it to contract.ts and updated viem version (yes we need to do that in challenges), it seems to work for useScafffoldEventSubscriber :
Screenshot 2023-09-29 at 9 37 07 PM

@rin-st
Copy link
Member Author

rin-st commented Sep 29, 2023

I think we missed updating contract.ts in that PR looking at files changed

I did it intentionally since without that pr people could be blocked from completing the challenges and I thought something wrong with types. I'll update it in challenges.

Merging this, thanks!

@rin-st rin-st merged commit c464451 into main Sep 29, 2023
1 check passed
@rin-st rin-st deleted the feat/add-indexed-params-to-events branch September 29, 2023 17:00
@github-actions github-actions bot mentioned this pull request Oct 5, 2023
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.

3 participants