Skip to content

Conversation

@jmg-duarte
Copy link
Contributor

@jmg-duarte jmg-duarte commented Nov 28, 2025

Description

Migrates the fetch_skips_failed_allowance_calls test to alloy, directly addressing this comment
#3927 (comment)

Changes

  • Migrates the fetch_skips_failed_allowance_calls test to alloy
  • Removes the last usages of ethcontract/web3 for crates/solver/src/interactions/allowances.rs

How to test

Existing tests

@jmg-duarte jmg-duarte requested a review from a team as a code owner November 28, 2025 11:31
let asserter = Asserter::new();
let provider = ProviderBuilder::new().connect_mocked_client(asserter.clone());

asserter.push_success(&U256::from(1337).abi_encode());
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunate that we apparently can't assert anything on the request but this logic is called a lot during e2e tests anyway.

.expect(&format!("should have a spender key {:?}", spender));
assert_eq!(allowances.spender, spender);
// We don't check which of the two (0x11, 0x22) got the error vs the result
// because it's order dependent and without a custom mock transport just for
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be possible to build the hashset with a seeded hasher to make the order stable, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The result hashmap is built inside and that's the one we check against, so I don't think so? Unless you mean changing the fetch_allowances code

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC we primarily need to ensure that the requests we pass into fetch_allowances are handled in a deterministic fashion. The out hashmap only has 1 entry so order doesn't matter there and if the inner hashset (line 384) has stable ordering the resulting RPC requests should be sent to the mock provider in a deterministic order as well.
The order of the result hashmap doesn't matter since we can just look up the results with the key.
OTOH this relies on implementation details so changing those might result in a failing tests even if the output might not change.

Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

Fine to merge after fixing the remaining lint.

@jmg-duarte jmg-duarte enabled auto-merge November 28, 2025 12:27
@jmg-duarte jmg-duarte added this pull request to the merge queue Nov 28, 2025
Merged via the queue into main with commit d1ef8f5 Nov 28, 2025
18 checks passed
@jmg-duarte jmg-duarte deleted the jmgd/alloy/mock branch November 28, 2025 12:54
@github-actions github-actions bot locked and limited conversation to collaborators Nov 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants