Skip to content

Conversation

@techvoyagerX
Copy link
Contributor

Fix: Handle Async Signature and Explicit Address Retrieval in ERC2771Context Tests

Overview

This PR improves the tests for the ERC2771Context contract by addressing asynchronous behavior and enhancing clarity in event comparisons.

Changes Introduced

  • Async Handling:
    All calls to signTypedData are now properly awaited to avoid potential race conditions during signature generation.

  • Explicit Address Retrieval:
    Event comparisons have been updated to explicitly use await sender.getAddress(), ensuring that the expected values are in the correct format.

  • Comment Cleanup:
    Fixed a minor typo in a comment from “poisonned” to “poisoned” for clarity.

@changeset-bot
Copy link

changeset-bot bot commented Feb 10, 2025

⚠️ No Changeset found

Latest commit: 83a7d86

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

Some of the proposed change may go in the right direction, but the diff is really messy.

Many blank lines are removes without reasons. Some changes make things more messy for no reason (.withArgs(this.sender); works just fine, no need to change it to .withArgs(await this.sender.getAddress());)

@techvoyagerX techvoyagerX requested a review from Amxx February 11, 2025 13:35
deadline: MAX_UINT48,
};

req.signature = this.sender.signTypedData(this.domain, this.types, req);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this used to be a change (adding the await) and now its not

@Amxx
Copy link
Collaborator

Amxx commented Feb 12, 2025

It looks like some of the good changes were reverted in daee720, but at the same time all of the "remove empty lines" was kept :/

Amxx
Amxx previously approved these changes Feb 12, 2025
@Amxx Amxx requested review from arr00 and ernestognw February 12, 2025 12:59
@techvoyagerX
Copy link
Contributor Author

can you address this PR asap? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants