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

Make eth_accounts return impersonated accounts #5734

Merged
merged 2 commits into from
Aug 27, 2023

Conversation

MartinquaXD
Copy link
Contributor

Motivation

Fix for #5732 (see that issue for detailed context).

Solution

Added CheatsManager::impersonated_accounts() to allow fetching impersonated accounts from EthApi.
Collected relevant addresses in EthApi::accounts() into a HashSet to avoid returning duplicate accounts when a user tries to impersonate an account that is already owned by the node.
Extended the can_impersonate_account unit test to assert that the impersonated account gets returned by the eth_accounts handler.

@MartinquaXD MartinquaXD changed the title Return impersonated accounts Make eth_accounts return impersonated accounts Aug 26, 2023
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

supportive

Comment on lines 531 to 535
let mut accounts = HashSet::new();
for signer in self.signers.iter() {
accounts.append(&mut signer.accounts());
accounts.extend(signer.accounts());
}
Ok(accounts)
accounts.extend(self.backend.cheats().impersonated_accounts());
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to keep a consistent order here and still use vec, but use an additional hashmap to filter unique addresses,
accounts.extend(signer.accounts().filter(|acc| unique.insert(*acc)))

Copy link
Contributor Author

@MartinquaXD MartinquaXD Aug 27, 2023

Choose a reason for hiding this comment

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

I implemented what I think you suggested: bffe055
However, this ended up a bit verbose because the filter needs to be applied to both iterators. If you don't care about listing the original accounts first and the impersonated accounts later an alternative implementation could also be this:

        let mut accounts = Vec::new();
        for signer in self.signers.iter() {
            accounts.append(&mut signer.accounts());
        }
        accounts.extend(self.backend.cheats().impersonated_accounts());
        accounts.sort_unstable();
        accounts.dedup();

LMK what you prefer.

Copy link
Contributor Author

@MartinquaXD MartinquaXD Aug 27, 2023

Choose a reason for hiding this comment

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

I'd like to keep a consistent order here

Actually since impersonated_accounts returns a HashMap the iterator produced by that already has an unstable order if you are impersonating > 1 accounts.

@mattsse mattsse merged commit bff4ed9 into foundry-rs:master Aug 27, 2023
15 checks passed
mikelodder7 pushed a commit to LIT-Protocol/foundry that referenced this pull request Sep 12, 2023
* Return impersonated accounts

* Return unique accounts in deterministic order
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.

2 participants