-
Notifications
You must be signed in to change notification settings - Fork 284
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
Check if ETHEREUM_4337_BUNDLER_URL uses the correct chainId #2013
Conversation
Uxio0
commented
Apr 28, 2024
- Closes Check if ETHEREUM_4337_BUNDLER_URL uses the same chainId as ETHEREUM_NODE_URL #1987
e06fe1e
to
c5c256b
Compare
""" | ||
if settings.ETHEREUM_4337_BUNDLER_URL: | ||
return BundlerClient(settings.ETHEREUM_4337_BUNDLER_URL) | ||
logger.warning("ETHEREUM_4337_BUNDLER_URL not set, cannot configure bundler client") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to remove the redundant warning after call this cached function here https://github.com/safe-global/safe-transaction-service/pull/2013/files#diff-2061896cceba9e91196c8a52412e1843ee140f0994f24987f02734187d40e7f9R48
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One warning is for the bundler client, other is for initializing the AccountAbstractionService
, I don't think they are redundant
bundler_client = get_bundler_client() | ||
if bundler_client: | ||
bundler_chain_id = bundler_client.get_chain_id() | ||
if bundler_chain_id == chain_id: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not uses the database chain.chain_id
field instead the RPC value?
For me is clearer to compare with database than a field that was previously checked with database.
I mean, database is the source of truth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
c5c256b
to
56cb2ad
Compare
56cb2ad
to
90730cf
Compare