-
Notifications
You must be signed in to change notification settings - Fork 320
V1.5.0 support #2714
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
V1.5.0 support #2714
Conversation
2af08a2 to
1513b63
Compare
safe_transaction_service/history/indexers/safe_events_indexer.py
Outdated
Show resolved
Hide resolved
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.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
safe_transaction_service/account_abstraction/tests/test_helpers.py
Outdated
Show resolved
Hide resolved
safe_transaction_service/history/indexers/proxy_factory_indexer.py
Outdated
Show resolved
Hide resolved
safe_transaction_service/history/indexers/safe_events_indexer.py
Outdated
Show resolved
Hide resolved
safe_transaction_service/history/tests/test_safe_events_indexer.py
Outdated
Show resolved
Hide resolved
96ec7d6 to
9d43ef5
Compare
58d920c to
044a986
Compare
| ) | ||
| parsed_signatures = SafeSignature.parse_signature( | ||
| signature, safe_tx_hash, safe_hash_preimage=safe_tx.safe_tx_hash_preimage | ||
| signature, safe_tx_hash, safe_hash_preimage=safe_signature_hash |
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.
Delegate signature validation missing v1.5.0 preimage handling
The validate_delegator_signature method unconditionally passes preimage as safe_hash_preimage when parsing signatures, but doesn't apply the v1.5.0 version-specific handling that was added elsewhere in this PR. For v1.5.0+ Safes using EIP-1271 signatures, the isValidSignature function expects the original message_hash rather than the Safe-encoded preimage. The transaction and message signature flows were updated to use select_safe_encoded_message_hash_by_safe_version, but the delegate signature flow was not. This causes delegate signatures from v1.5.0 Safe signers using EIP-1271 to fail validation.
| """ | ||
| if Version(safe_version) >= Version("1.5.0"): | ||
| return safe_message_hash | ||
| return safe_message_preimage |
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.
Missing null check for safe version in version comparison
The select_safe_encoded_message_hash_by_safe_version function passes safe_version directly to Version() without null checking. Safe.get_version() can return None for very old Safe contracts or non-standard implementations. When None is passed to Version(), it raises a TypeError, causing API requests to fail with a 500 error. The function assumes safe_version is always a valid string but callers pass safe.get_version() which may be None.
Additional Locations (2)
Done here fadb2da |
safe_transaction_service/history/indexers/safe_events_indexer.py
Outdated
Show resolved
Hide resolved
| self.assertEqual(safe_contract_delegate.delegator, safe_owner.address) | ||
| self.assertEqual(safe_contract_delegate.safe_contract_id, nested_safe.address) | ||
|
|
||
| # TODO: Now that we have TestViewsV2V141 and TestViewsV2V150, should be create TestViewsV2V130? |
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 think it would work very similarly to 141, so I wouldn't add it at first. The TODO can be removed
| ) | ||
| parsed_signatures = SafeSignature.parse_signature( | ||
| signature, safe_tx_hash, safe_hash_preimage=safe_tx.safe_tx_hash_preimage | ||
| signature, safe_tx_hash, safe_hash_preimage=safe_signature_hash |
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.
Delegate EIP-1271 signatures lack v1.5.0 version-specific hash handling
The delegate signature validation in _validate_delegate_v2_signature always passes preimage as safe_hash_preimage to SafeSignature.parse_signature, without applying version-specific handling. For v1.5.0+ Safes, isValidSignature expects the original message hash instead of the Safe-encoded preimage. Transaction signatures and safe message signatures were updated to use select_safe_encoded_message_hash_by_safe_version, but delegate signatures were not. EIP-1271 delegate signatures from v1.5.0+ Safe owners will fail validation because the wrong hash format is passed to isValidSignature.
| safe.get_version(), safe_message_hash, safe_message_preimage | ||
| ) | ||
| safe_signatures = SafeSignature.parse_signature( | ||
| signature, safe_message_hash, safe_hash_preimage=safe_message_preimage |
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.
After reviewing the changes more carefully, I realized they are incorrect and introduce an unnecessary RPC call to fetch the Safe version.
parse_signature does not validate signatures; it only parses them and returns Signature objects (in this case, ContractSignature). Overwriting safe_message_preimage with safe_message_hash therefore just duplicates the field and adds no validation.
Signature validation actually happens in get_valid_owner_from_signatures, specifically on safe_signature of safe-eth-py here:
https://github.com/safe-global/safe-eth-py/blob/647ad2e79f347ae72693d34ac274e9047e99c3e9/safe_eth/safe/safe_signature.py#L450
| ) | ||
|
|
||
| safe_owners = get_safe_owners(safe_address) | ||
| safe_hash_preimage = select_safe_encoded_message_hash_by_safe_version( |
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 think that we don'r need this function select_safe_encoded_message_hash_by_safe_version
| ) | ||
| parsed_signatures = SafeSignature.parse_signature( | ||
| signature, safe_tx_hash, safe_hash_preimage=safe_tx.safe_tx_hash_preimage | ||
| signature, safe_tx_hash, safe_hash_preimage=safe_hash_preimage |
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.
parse_signature does not validate signatures; it only parses them and returns Signature objects (in this case, ContractSignature). Overwriting safe_message_preimage with safe_message_hash therefore just duplicates the field and adds no validation.
Signature validation actually happens in get_valid_owner_from_signatures, specifically on safe_signature of safe-eth-py here:
https://github.com/safe-global/safe-eth-py/blob/647ad2e79f347ae72693d34ac274e9047e99c3e9/safe_eth/safe/safe_signature.py#L450
|
|
||
| class TestAccountAbstractionHelpers(SafeTestCaseMixin, TestCase): | ||
| def test_decode_init_code(self): | ||
| def test_decode_init_code_v141(self): |
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.
This changes looks unnecessary.
- Ensure that 1271 works from 1_1_1.
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.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| master_copy = EthereumAddressField() | ||
| fallback_handler = EthereumAddressField() | ||
| guard = EthereumAddressField(allow_null=True) | ||
| module_guard = EthereumAddressField() |
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.
Missing allow_null=True for module_guard serializer field
The module_guard field in SafeLastStatusSerializer is defined as EthereumAddressField() without allow_null=True, but the database column (added in migration 0100) has null=True and default=None. After migration, existing SafeLastStatus records will have module_guard=None. When OwnersViewV2 or ModulesViewV2 endpoints try to serialize these records, the serializer will fail because it doesn't accept null values. Compare with the guard field on line 927 which correctly has allow_null=True.
I synced with Uxio previously to his holidays.
Safe v1.5.0 Support
This PR implements support for Safe Smart Account v1.5.0 while maintaining backward compatibility with v1.4.1. The transaction service now supports both versions simultaneously.
Changes
Database Schema
guardfield totransaction_guardinSafeStatusandSafeLastStatusmodelsmodule_guardfield toSafeStatusandSafeLastStatusmodels0096_guard_changes_v150.pyEvent Indexing
ChangedModuleGuardevent in Safe v1.5.0setModuleGuardfunction handler in transaction processorAccount Abstraction
decode_init_codehelper to support both v1.4.1 and v1.5.0 proxy factoriesget_contract_instancesfunction that dynamically selects appropriate factory and contract versions based on factory addressAPI Changes
SafeInfoResponseSerializerto include:transaction_guard(renamed fromguard)module_guard(new field)SafeInfo.get_safe_info()to correctly map fields to SafeInfo constructor parametersTest Updates
Testing Gaps
decode_init_codetest for v1.5.0 is currently not implemented because safe-eth-py lacks mock UserOperation data with initCode with v1.5.0 proxy factoryTODO
safe-eth-py. The PR cannot be merged/reviewed until a new safe-eth-py version is published with v1.5.0 support.ExtensibleFallabackHandlerevents to indexing. Should we even do it?Answer: As discussed with team, we are not adding it now.
test_views.pyandtest_views_v2.pyrequirements.txtNote
Introduces full Safe v1.5.0 compatibility while preserving support for prior versions.
module_guardtoSafeStatus/SafeLastStatus(migration0100_*), expose inSafeInfoResponseSerializer/SafeLastStatusSerializer, show/filter in admin.ChangedModuleGuard; map tosetModuleGuard; include v1.5.0 failure/module-failure events; refine creation flow (ProxyCreationL2 note).TxDecoder/SafeTxDecoder.ProxyFactory.from_addresswith version detection andget_safe_contract_by_version; clearer error on unknown factory.safe-eth-pyto7.17.1.Written by Cursor Bugbot for commit b6d1083. This will update automatically on new commits. Configure here.