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

feat: optimize trusted contracts retrieval #2430

Merged
merged 4 commits into from
Mar 7, 2025

Conversation

hectorgomezv
Copy link
Member

@hectorgomezv hectorgomezv commented Mar 6, 2025

Summary

After the addition of a new filter that makes it possible to get the complete list of trusted for delegate call contracts in the Transaction Service API, the Client Gateway can take advantage of it by caching that list and comparing proposals and transaction mappings against the list, instead of checking each contract on demand.

This PR adds a new function to retrieve the list, and checks against that list instead of calling ContractRepository.getContract in both TransactionDataMapper and SafeRepository.

Changes

  • Adds the ContractRepository['isTrustedForDelegateCall'] function.
  • Adds an FF_TRUSTED_FOR_DELEGATE_CALL_CONTRACTS_LIST feature flag.
  • Adjusts the related tests.

@hectorgomezv hectorgomezv self-assigned this Mar 6, 2025
@hectorgomezv hectorgomezv requested a review from a team as a code owner March 6, 2025 15:24
args.chainId,
);
return trustedContracts.results.some((contract) =>
isAddressEqual(contract.address, args.contractAddress),
Copy link
Member

Choose a reason for hiding this comment

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

Althought theoretically correct, I think we should still check the flag to be safe.

Suggested change
isAddressEqual(contract.address, args.contractAddress),
isAddressEqual(contract.address, args.contractAddress) &&
contract.trustedForDelegateCall,```

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Even if it's not needed theoretically, we can check the flag to avoid the case the filter is not working properly on the Transaction Service for some reason. Thanks. Changed in 7e3b393

Copy link
Member

Choose a reason for hiding this comment

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

Should we want to expose pages of contracts later on, we are also covered, e.g. we call a hypothetical getContracts method on the domain, passing the query there instead.

@hectorgomezv hectorgomezv requested a review from iamacook March 7, 2025 08:48
@hectorgomezv hectorgomezv merged commit 299b390 into main Mar 7, 2025
16 checks passed
@hectorgomezv hectorgomezv deleted the feat/optimize-trusted-contracts-retrieval branch March 7, 2025 08:59
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