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

fix(relayer): Ensure argument keys are sorted before hashing #2061

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

pxrl
Copy link
Contributor

@pxrl pxrl commented Feb 7, 2025

Unclear whether this has actually caused an issue, but implementation details at the upstream RPC provider may lead to undetected duplicate events being passed into the relayer.

Unclear whether this has actually caused an issue, but implementation
details at the upstream RPC provider may lead to undetected duplicate
events being passed into the relayer.
nicholaspai
nicholaspai previously approved these changes Feb 7, 2025
@pxrl pxrl added the do not merge Don't merge until label is removed label Feb 11, 2025
@pxrl
Copy link
Contributor Author

pxrl commented Feb 11, 2025

Marked as do not merge for now; this current iteration doesn't work well with fill events where the event contains nested objects.

@bmzig
Copy link
Contributor

bmzig commented Feb 11, 2025

To be fair, we probably don't need this anymore, right? Do we still want it only if there are weird RPC quirks in the future?

@pxrl
Copy link
Contributor Author

pxrl commented Feb 11, 2025

To be fair, we probably don't need this anymore, right? Do we still want it only if there are weird RPC quirks in the future?

Yeah..I don't think we've been impacted by this, but in general it's probably a good idea to be consistent in how these events are handled, so I'd like to apply sorting.

@pxrl pxrl removed the do not merge Don't merge until label is removed label Feb 13, 2025
@pxrl
Copy link
Contributor Author

pxrl commented Feb 13, 2025

@bmzig wdyt about this change?

Copy link
Contributor

@bmzig bmzig 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 to me.

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