Skip to content

refactor: Remove unused contract address fields from chain config#1516

Open
Zalens wants to merge 2 commits intomainfrom
valens/pla-1142-remove-unused-contract-address-fields-from-chain-config
Open

refactor: Remove unused contract address fields from chain config#1516
Zalens wants to merge 2 commits intomainfrom
valens/pla-1142-remove-unused-contract-address-fields-from-chain-config

Conversation

@Zalens
Copy link
Copy Markdown
Member

@Zalens Zalens commented Mar 25, 2026

Summary

  • Removes 9 unused contract address fields (safeSingletonAddress, safeProxyFactoryAddress, multiSendAddress, multiSendCallOnlyAddress, fallbackHandlerAddress, signMessageLibAddress, createCallAddress, simulateTxAccessorAddress, safeWebAuthnSignerFactoryAddress) from the Chain model and API response
  • These fields were added in July 2024 but never populated — they appear empty on both staging and production
  • Removes the ContractAddressesSerializer, model fields, test factory assignments, and test expectations
  • Includes a migration to drop the columns from the database

Closes PLA-1142

Test plan

  • All 235 existing tests pass
  • Verify staging API response no longer includes contractAddresses
  • Confirm no downstream clients depend on these fields

🤖 Generated with Claude Code

These 9 contract address fields were added in July 2024 but never
populated with data. They appear empty on staging and production,
adding noise to the API response without serving any purpose.

Closes PLA-1142

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Zalens Zalens requested a review from a team as a code owner March 25, 2026 09:49
@coveralls
Copy link
Copy Markdown

coveralls commented Mar 25, 2026

Pull Request Test Coverage Report for Build 23534977119

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.006%) to 99.576%

Totals Coverage Status
Change from base Build 23254455469: -0.006%
Covered Lines: 1410
Relevant Lines: 1416

💛 - Coveralls

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@compojoom
Copy link
Copy Markdown

@Zalens do we also need a PR on the client-gateway? The chain object there has those fields as well.

@Zalens
Copy link
Copy Markdown
Member Author

Zalens commented Mar 25, 2026

@Zalens do we also need a PR on the client-gateway? The chain object there has those fields as well.

Good point, I'll check

Zalens added a commit to safe-global/safe-client-gateway that referenced this pull request Mar 25, 2026
Remove ContractAddressesSchema, domain entity, route DTO, test builder,
and all references to contractAddresses from chain objects. These fields
were never populated in the config service and are being removed there
as well (safe-global/safe-config-service#1516).

Closes PLA-1142

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Zalens
Copy link
Copy Markdown
Member Author

Zalens commented Mar 25, 2026

@PooyaRaki
Copy link
Copy Markdown
Contributor

@claude review

@claude
Copy link
Copy Markdown

claude bot commented Apr 16, 2026

Claude finished @PooyaRaki's task —— View job


Review

  • Read changed files
  • Check for stale references to removed fields
  • Analyze code quality, bugs, security, performance, test coverage

Looks good. Clean removal across model, serializer, factory, and tests. The migration is correct and the dependency chain is proper (00540055).

One minor note:

  • No admin.py cleanup needed — confirmed no references to these fields exist in admin configuration.
  • No stale references — grep across src/ confirms only the old migration files (0043, 0044, 0046, 0047) still reference these field names, which is expected and correct (migration history must be preserved).
  • Test coverage is adequate — the test_json_payload_format tests for both list and detail views confirm contractAddresses is no longer in the response.

No issues found. Approve.


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.

4 participants