Repository navigation
Sync Safe config with api-kit 5.0.3 and add ADI Chain - #32
Conversation
Mirrors the api-kit 5.0.3 network list, which adds Pharos, Robinhood, Arc and their testnets among others, and drops zkEVM, BTC and Codex. ADI Chain (36900) is served by the official transaction service under `adi` but is not yet in the api-kit config, so it is added beside the mirrored list.
5e6d47c to
a3de4f7
Compare
aviggiano
left a comment
There was a problem hiding this comment.
Thanks for syncing this! One blocking issue: dropping Polygon zkEVM (1101), BTC (3637) and Codex (81224) from the MultiSendCallOnly lookups breaks batch simulation and calldata building on those chains, even though the contract is still deployed there. Only the tx service went away. The multisend FFI test is scoped to api-kit's chain list, so CI doesn't catch it. Details and smaller suggestions are inline.
| @@ -151,16 +166,19 @@ library Safe { | |||
|
|
|||
| function _usesV130CanonicalMultiSend(uint256 chainId) private pure returns (bool) { | |||
| return (chainId == 1 || chainId == 10 || chainId == 56 || chainId == 100 || chainId == 130 || chainId == 137 | |||
There was a problem hiding this comment.
Removing 1101 / 3637 / 81224 from the MultiSendCallOnly lookups is a regression. These chains were dropped from api-kit because the Safe tx service no longer serves them (/tx-service/zkevm, /btc, /codex all return 404). But MultiSendCallOnly is still deployed there: safe-deployments 1.37.63 lists all three as canonical in both v1.3.0 and v1.4.1 multi_send_call_only.json.
simulateTransactionsNoSign, simulateTransactionsMultiSigNoSign and getProposeTransactionsTargetAndData never touch the tx service. They only call getMultiSendCallOnly(block.chainid), which will now revert with MultiSendCallOnlyNotFound(1101) (and likewise for 3637 / 81224). That breaks offline batch simulation, and calldata building for anyone who submits outside the API.
Suggest keeping these chains in _usesV130CanonicalMultiSend / _usesV141CanonicalMultiSend (and in multiSendChains()), and dropping them only from getNetworkShortName.
There was a problem hiding this comment.
Good catch, I didn't know the contract outlived the service on those. Restored all three; only the tx-service lookup drops them now.
| } | ||
|
|
||
| for (const chainId of SUPPORTED_THIRD_PARTY_CHAIN_IDS) { | ||
| for (const chainId of [...SUPPORTED_UNLISTED_OFFICIAL_CHAIN_IDS, ...SUPPORTED_THIRD_PARTY_CHAIN_IDS]) { |
There was a problem hiding this comment.
The multisend differential test only iterates chains api-kit lists (plus the hand-added extras). So when api-kit drops a chain, its still-valid multisend mapping silently stops being checked. That's why removing 1101/3637/81224 from Safe.sol passes CI. Consider driving the multisend check from the safe-deployments networkAddresses, or at least from the union of the previous fixture set and api-kit, so removals are caught.
There was a problem hiding this comment.
Done. The test now hands the fixture's chain ids to the script, which checks the union with api-kit's list, so a chain api-kit drops stays checked as long as the library resolves it. Removing one of the three now fails both tests.
| if (chainId == 16661) return "0g"; | ||
| if (chainId == 25363) return "fluent"; | ||
| // Served by the official transaction service, not yet listed in the api-kit config. | ||
| if (chainId == 36900) return "adi"; |
There was a problem hiding this comment.
The function header says this mirrors api-kit config.ts, but adi is hand-added inside the mirrored table. The next person who regenerates the list from api-kit (as the comment instructs) will silently drop it, and ADI users will get ApiKitUrlNotFound(36900). Suggest moving it to a separate override block next to the Plume special case in getApiKitUrl, so the mirrored table stays an exact mirror.
There was a problem hiding this comment.
Moved out. The mirror is now a private function that's an exact copy of api-kit's table, and ADI sits in an override on top of it.
| index = _pushMultiSend(chainIds, expected, index, 11142220, MULTI_SEND_CALL_ONLY_ADDRESS_V141_CANONICAL); | ||
| index = _pushMultiSend(chainIds, expected, index, 11155111, MULTI_SEND_CALL_ONLY_ADDRESS_V130_CANONICAL); | ||
| index = _pushMultiSend(chainIds, expected, index, 1313161554, MULTI_SEND_CALL_ONLY_ADDRESS_V130_CANONICAL); | ||
| index = _pushMultiSend(chainIds, expected, index, 36900, MULTI_SEND_CALL_ONLY_ADDRESS_V141_CANONICAL); |
There was a problem hiding this comment.
36900 is added to multiSendChains() but not to officialChains(), so test_Safe_getTransactionServiceUrl_matchesLatestOfficialSdkConfig never sweeps it. Only the single ad-hoc test covers it. Also, once api-kit adds ADI, the FFI script will throw Duplicate supported chainId: 36900, and both fixture files will need updating together. A dedicated unlistedOfficialChains() fixture used by both tests would keep this consistent.
There was a problem hiding this comment.
Added unlistedOfficialChains(). The URL test sweeps it with the same helper as the official list, and the script no longer hardcodes it, so when api-kit adds ADI the differential test just asks to move it over.
| @@ -4,6 +4,8 @@ const vm = require("vm"); | |||
|
|
|||
| const MIN_EXPECTED_NETWORKS = 39; | |||
There was a problem hiding this comment.
MIN_EXPECTED_NETWORKS is still 39, but api-kit 5.0.3 exposes 52 networks. A truncated or partial regex match of ~40 entries would still pass this sanity check, and the differential test would then quietly cover only a subset. Suggest bumping to 52 here and in test/ffi/safe-api-kit-config.cjs.
There was a problem hiding this comment.
Bumped to 52 in both scripts.
| "devDependencies": { | ||
| "@safe-global/api-kit": "4.1.0", | ||
| "@safe-global/safe-deployments": "1.37.53" | ||
| "@safe-global/api-kit": "5.0.3", |
There was a problem hiding this comment.
This is a major bump (4.1.0 → 5.0.3), and both FFI scripts depend on regex-scraping var networks = [...] out of the built bundle. That coupling to bundler output is fragile across majors. Not blocking if it works today, but worth either pinning tightly with a comment or switching to a stable exported API if 5.x provides one.
There was a problem hiding this comment.
Checked: 5.0.3 exports only the client and HttpError, no network list, so the scrape has to stay. It's pinned exactly and both scripts now say why.
| || chainId == 84532 || chainId == 534352 || chainId == 11155111 || chainId == 1313161554); | ||
| } | ||
|
|
||
| function _usesV141CanonicalMultiSend(uint256 chainId) private pure returns (bool) { |
There was a problem hiding this comment.
Nit: this is now a 31-term || chain that must be hand-synced with getNetworkShortName and two fixture arrays with hardcoded lengths. Nearly every new chain is v1.4.1, so it might be simpler to keep the v1.3.0 / zkSync exceptions as the explicit lists and default supported chains to v1.4.1. Forgetting a chain here currently means it resolves a tx-service URL but batch proposals revert with MultiSendCallOnlyNotFound.
There was a problem hiding this comment.
Did it. v1.3.0 and zkSync stay explicit, and everything else with a tx service defaults to v1.4.1. Plume, BTC and Codex are the only extra v1.4.1 entries.
| instance(self).requestBody = vm.serializeUint(".proposeTransaction", "nonce", params.nonce); | ||
|
|
||
| HTTP.Response memory response = instance(self).http.instance() | ||
| HTTP.Response memory response = instance(self) |
There was a problem hiding this comment.
Nit: formatting-only change (probably a different forge fmt version). It adds noise to a config-sync PR and may flip back and forth between contributors on different forge versions.
There was a problem hiding this comment.
That one is needed, sadly: CI installs stable, which is 1.8.5 now, and it fails forge fmt --check on the old layout. 1.4.x wants the old one back, so whoever formats next needs the current stable.
Polygon zkEVM, BTC and Codex left api-kit because the official transaction service dropped them, but safe-deployments still lists the contract there, so only the service lookup forgets them. Every other chain with a service now defaults to the canonical v1.4.1 contract, with the v1.3.0 and zkSync chains as the explicit lists, and ADI sits in an override above an exact api-kit mirror. The multisend differential test sweeps the union of api-kit's list and the fixture, so a chain api-kit drops stays checked for as long as the library resolves it.
|
Thanks for the fast answer! Applying the fixes and suggestion, didn't know about the removed chains! |
Summary
@safe-global/api-kitto 5.0.3 and@safe-global/safe-deploymentsto 1.37.63, and mirror the new network list inSafe.sol: adds Pharos (1672), Robinhood (4663), Arc (5042), their testnets and the other chains api-kit gained; drops Polygon zkEVM (1101), BTC (3637) and Codex (81224), which api-kit no longer serves.adislug. The official transaction service answers for it and indexes Safes there, but api-kit does not list it yet, so it sits beside the mirrored list with a comment, the same way Plume sits outside it.MultiSendCallOnly, which safe-deployments records for each of them.Testing
forge test(29 passed, including the differential tests against api-kit 5.0.3 and safe-deployments 1.37.63)forge fmt --check