Sync Safe config with latest safe-core-sdk networks

[aviggiano]

Summary

• mirror the latest safe-core-sdk api-kit network short-name config in Safe.sol
• separate official Safe transaction-service URLs from explicit third-party overrides like Plume
• add unit coverage for official network URL resolution, third-party overrides, and multisend address selection

Testing

• PATH=/home/ubuntu/.foundry/bin:$PATH /home/ubuntu/.foundry/bin/forge test

[aviggiano]

Code Review

Nice refactor — moving from per-instance storage mappings to pure functions is a meaningful gas improvement (eliminates all the SSTORE/SLOAD overhead for static config). The differential FFI test against the actual @safe-global/api-kit bundle is a clever way to keep the Solidity config in sync.

Questions / issues

1. Lens (chainId 232) uses MULTI_SEND_CALL_ONLY_ADDRESS_V141_ZKSYNC — is this intentional? Lens isn't a ZKSync-derived chain (unlike chain 324). If this comes from the safe-deployments repo, a brief comment explaining why it uses the ZKSync address would prevent future confusion.

2. Multisend test coverage is thin. SafeConfigFixtures.multiSendChains() only tests 5 out of 40 chains. The differential FFI test validates URL resolution exhaustively, but multisend address resolution is only spot-checked. Consider expanding the fixture to cover all chains, or adding a differential test that pulls multisend addresses from the safe-deployments JSON as well.

3. FFI script regex fragility — safe-api-kit-config.cjs parses the SDK bundle with regexes like /var TRANSACTION_SERVICE_URL = "([^"]+)";/ and /var networks = (\[[\s\S]*?\n\]);\nvar getNetworkShortName =/. If the SDK changes its bundler or variable names, these will break silently (the throw catches total misses, but a partial/corrupt match could produce wrong data). Maybe worth a sanity check like asserting that the extracted networks array has >= N entries.

4. vm.runInNewContext on bundle content — low risk since it's a pinned dev dependency, but worth calling out: if @safe-global/api-kit is ever compromised, this executes arbitrary code during forge test. Pinning to an exact version (which you've done: 4.1.0) mitigates this well.

5. Minor: unnamed storage parameter style. getApiKitUrl(Client storage, uint256 chainId) and getMultiSendCallOnly(Client storage, uint256 chainId) — the unnamed Client storage param is kept for API compatibility but is now unused. If there are no external callers relying on the signature, consider removing it to simplify the interface. If kept for backwards compatibility, a comment explaining why would help.

Looks good

• Struct slimming (removing the two mappings from Instance) is clean
• Versioned multisend constants (V130 vs V141) with explicit chain-to-version mapping is more accurate than the old blanket approach
• Test structure is well-organized (unit tests, differential tests, fixtures)
• CI change to add Node setup is straightforward

Overall LGTM with the Lens/ZKSync question clarified and ideally broader multisend test coverage.

[aviggiano]

I found a functional issue in the new getMultiSendCallOnly table.

• src/Safe.sol:152-156 puts chain 10200 in _usesV141CanonicalMultiSend(), so getMultiSendCallOnly(10200) resolves to 0x9641d764fc13c8B624c04430C7356C1C7C8102e2.
• But the exact safe-deployments commit referenced in this PR (c6a2025fca317b629d73d24b472c266418e2a4d6) only registers chain 10200 in src/assets/v1.3.0/multi_send_call_only.json; it is absent from src/assets/v1.4.1/multi_send_call_only.json.
• The same source shows the inverse problem for Plume: chain 98866 only appears in v1.4.1, but src/Safe.sol:127-129 and test/helpers/SafeConfigFixtures.sol:56-60 still pin it to the old v1.3.0 canonical address 0x40A2aCCbd92BCA938b02010E17A5b8929b49130D.

Because getProposeTransactionsTargetAndData() uses this lookup for batched transactions, these mismatches can send batch proposals to the wrong MultiSendCallOnly address on Chiado and Plume. I’d fix the two outliers before merging and extend the multisend fixture so the tests cover them explicitly.

[aviggiano]

Re #29 (review)

Reproduced locally. The two outliers were real against safe-global/safe-deployments at c6a2025fca317b629d73d24b472c266418e2a4d6, and they are fixed in 2257730.

• 10200 moved from the v1.4.1 canonical bucket to the v1.3.0 canonical bucket.
• 98866 now resolves to the v1.4.1 canonical multisend address.
• The multisend fixture was expanded to all supported chains so both outliers are covered explicitly in the unit test loop.

Local verification: PATH=/home/ubuntu/.foundry/bin:$PATH /home/ubuntu/.foundry/bin/forge test --match-contract 'Safe(Test|ConfigTest|DifferentialTest)'

[aviggiano]

Re #29 (review)

Tested each point locally against the current branch and, where relevant, against a checkout of safe-global/safe-deployments at c6a2025fca317b629d73d24b472c266418e2a4d6. Follow-up is in 2257730.

• Added an inline comment explaining why Lens (232) uses the zkSync deployment: that is how safe-deployments registers it in both v1.3.0 and v1.4.1.
• Expanded SafeConfigFixtures.multiSendChains() from 5 entries to all 40 supported chains, so the multisend unit test now exercises the whole table.
• Hardened test/ffi/safe-api-kit-config.cjs with sanity checks on array shape, minimum entry count, duplicate chain IDs, and invalid shortName / chainId values.
• Kept the unnamed Client storage parameter, but documented why: it preserves existing using Safe for * call sites.
• On vm.runInNewContext: no code change beyond the stricter sanity checks; the exact-version pin remains in place.

Local verification: PATH=/home/ubuntu/.foundry/bin:$PATH /home/ubuntu/.foundry/bin/forge test --match-contract 'Safe(Test|ConfigTest|DifferentialTest)'