feat(proxy): switch FWSS to the upstream ERC-8167 dispatcher atomically - #616
CodeWarriorr wants to merge 63 commits into
Conversation
|
The PR mostly waits for #611 to be rebased, and its ready to review |
99b1769 to
aa70045
Compare
af1982b to
31e0d47
Compare
There was a problem hiding this comment.
I think we only ever call this with msg.sender, so best to remove the parameter
There was a problem hiding this comment.
if you convert LibAccessControl into a contract, you can share this modifier in the superclass
There was a problem hiding this comment.
done, LibAccessControl became FWSSOwnable
There was a problem hiding this comment.
don't need separate declarations per network. they can all go in the same file. there's a chainId key in deployments
There was a problem hiding this comment.
(for future proxies we'll have the same address for all networks)
There was a problem hiding this comment.
FWSS has different addresses on mainnet and calibnet though, and josuke uses the entry's address on every chain. How would you fit both in one file?
There was a problem hiding this comment.
Trivially. What do you think is the difficulty?
There was a problem hiding this comment.
merging is fine, what worries me is josuke deploy - it runs every entry against whatever chain its connected to, using that entry address. maybe we should embed chain id into the josuke config ?
There was a problem hiding this comment.
FYI: rebase rewrote you commit, sorry about that, content is unchanged of just the hash
There was a problem hiding this comment.
chainId is the key in deployments, and there's no support for initial deployments
There was a problem hiding this comment.
Kept both network entries in one josuke.json, as in your commit. The transition scripts select the proposed migration by the existing FWSS proxy address and deployments[chainId]. The body calls out the current deploy limitations; a real josuke deploy against the v1.4.0 proxy remains gated on wjmelements/josuke#3.
wjmelements
left a comment
wjmelements
left a comment
There was a problem hiding this comment.
This looks pretty good. As a follow-on task, we'll want to run most of the functional tests pre- and post-dispatcher.
Perhaps the view contract address(self) change can be a separate PR so we can merge this in sooner
1dc81f0 to
5ebd97e
Compare
|
Rebased on #611.
Pre/post-dispatcher tests are a follow-up once the business modules are in. |
Why? I wish you hadn't. |
85472b7 to
9a388f2
Compare
The monolith stays the implementation during the migration phase, so it keeps its full v1.4.0 interface. Scripts and synapse-sdk read the getter from the proxy and from the published ABI. The storage field becomes _viewContractAddress so a same-named getter can coexist with it. The layout snapshot drops the leading underscore, so the published label and VIEW_CONTRACT_ADDRESS_SLOT are unchanged. FWSSViewContractModule now inherits FWSSStorage instead of reading the slot.
|
resolve conflicts |
Squash-merged #611 restored the pre-rename provider module, its test and LibAccessControl; drop them in favour of FWSSProviderManagementModule and FWSSOwnable, which differ only in formatting and the owner base. Keep the base's commented ignored_error_codes and track FWSSMetadataModule's size.
…orge Exporting PATH inside make only reached make's own recipes, so josuke and shells never found the assembler. Installing it next to forge puts it on the PATH every Foundry user already has.
The FWSS monolith has 193 bytes of size headroom left, so it cannot keep carrying the one-shot dispatcher switch. ERC8167Transition holds only the UUPS entry point and the pinned dispatcher and migration; services supply authorization and the route check. FWSSDispatcherTransition pads itself past v1.4.0's 3000-byte announcement floor with distinct string chunks, which stay verifiable from source, unlike bytes appended at deployment.
The transition flows now upgrade the deployed v1.4.0 bytecode to the slim transition with migrate(migration). Re-entry reaches the dispatcher, since the transition uninstalls itself first, and a migration that replaces the dispatcher reverts. Tests of the monolith's own transition code stay until that code is removed.
FilecoinWarmStorageService goes back to the base implementation plus the _viewContractAddress rename: no transition constructor arguments, immutables or completeDispatcherTransition, and its ABI matches the base. FWSSDispatcherTransition carries the switch instead. The merge of #611 had kept this branch's deletion of addApprovedProvider and removeApprovedProvider; restore them with the base tests and deploy scripts, since the monolith stays complete during the migration. ERC8167Transition leaves src/lib/, whose events update-abi merges into the FWSS ABI.
josuke now generates and records every FWSS module migration, so its revision is pinned like the evm assembler's.
The transition pins the migration josuke deploy proposes in josuke.json. The new deploy script reads it from there, deploys the dispatcher if needed and records FWSSDispatcherTransition in deployments.json. The execute script detects the transition, checks its migration and code hash against the ledger, and sends migrate(migration); josuke accept then records the routes.
An upgrade to the transition without migrate data, followed by a migration that cannot complete, would leave the proxy with no way back. The transition now pins the implementation it replaces and abortTransition restores it.
The deploy script reads the current implementation for the transition's rollback target, deploys nothing under DRY_RUN and records checksummed addresses. The execute script checks the pinned previous implementation against the proxy before migrating.
cast unlocks ETH_KEYSTORE even for read-only calls, so with the keystore exported each read prompted for a password, or failed without a tty and was hidden by the stderr redirect. The plan check then reported an empty planned address.
There was a problem hiding this comment.
deploy using forge scripts, not bash scripts
| # cast unlocks ETH_KEYSTORE even for read-only calls, which prompts for a password or fails without a tty. | ||
| cast_call() { | ||
| env -u ETH_KEYSTORE cast call "$@" | ||
| } |
There was a problem hiding this comment.
I think the -f parameter used in every invocation here also prevents the password prompt
| JOSUKE_LEDGER="${JOSUKE_LEDGER:-$(dirname "${BASH_SOURCE[0]}")/../josuke.json}" | ||
|
|
||
| # Prints the EIP-55 migration `josuke deploy` proposed for a proxy on a chain, or nothing if there is none | ||
| # Args: $1=chain_id, $2=proxy_address |
There was a problem hiding this comment.
drop the first param and use $CHAIN
| * @notice Leaves the contract without an owner, disabling owner-only functions and migrations | ||
| */ | ||
| function renounceOwnership() external onlyOwner { | ||
| _transferOwnership(address(0)); | ||
| } |
There was a problem hiding this comment.
let's remove this function. it's dangerous. If we want to abdicate we would uninstall the onlyOwner methods instead
This branch has not been deployed
4 participants
Moves the FWSS proxy from the UUPS monolith to the unmodified upstream ERC-8167 dispatcher,
Proxy.evm, in one delayed upgrade call. Configured selectors route to their modules, and josuke deploys the modules and the migration that installs their routes. Part of #614, supersedes #615. Business modules come separately; don't run the transition on Calibration or Mainnet before they land.Why this approach. The dispatcher is upstream bytecode pinned by hash, with no FWSS code and no admin selectors. Live v1.4.0 can only change its implementation through its own delayed UUPS upgrade, and it rejects the raw dispatcher, which is 88 bytes with no
proxiableUUID. So a one-shot transition implementation carries josuke's migration through that upgrade. The normal upgrade call installs the dispatcher atomically; an empty-data upgrade can be completed or aborted by the owner. The monolith keeps no transition code and has 1590 bytes of size headroom.What's changed
ERC8167Transitionis a service-agnostic one-shot UUPS implementation. It pins the previous implementation, the dispatcher, the migration and the migration's code hash as immutables, so the announced delay covers exactly what will run.migrate(migration)points the proxy at the dispatcher first, then runs the migration in the proxy's storage, then checks routes. A service supplies authorization and the route check.FWSSDispatcherTransitionadds the FWSS owner check,LibUpgradeRoutes, the pinned dispatcher code hash and padding.announceUpgradePlanrequires over 3000 bytes of code; distinct string chunks, returned bylegacyUpgradePadding(), bring the transition to 3276 bytes. Repeated bytes get folded by the optimizer, and bytes appended at deployment would break source verification. A test pins the floor, and the transition tests announce it on the deployed v1.4.0 bytecode.upgradeToAndCall(transition, migrate(migration)). Any revert leaves v1.4.0 and its plan in place. The ordinarymigrate(viewContract)upgrade data reverts, so the regular execute script can't leave the transition installed. After an upgrade with empty data, the owner can still finish through the proxy, or callabortTransition()to restore the pinned previous implementation; that rollback is the only way out if the pinned migration can no longer complete.FWSSMigrateModuleruns later josuke migrations with the same owner check and delay, and rejects one that dropsimplementation,selectors,announceMigrationormigrate, or replaces the dispatcher.OwnershipModuleexposes the owner.FWSSViewContractModulereturns the view contract address.ExtsloadModulekeeps the deployed StateView working.FWSSOwnablereplacesLibAccessControl: an abstract contract with a sharedonlyOwnerand internal helpers, so facets get no extra selectors.josuke.jsonwith an entry per network. Offline tests resolvefacetSrc, reject duplicate selectors and install the facet set.make install-josukeinstalls the pinned CLI.warm-storage-deploy-dispatcher-transition.shreads josuke's proposed migration fromjosuke.json, reads the current implementation as the rollback target, deploys the dispatcher if needed and the transition, and supportsDRY_RUN.warm-storage-execute-upgrade.shdetects the transition, checks it against the ledger and the proxy's current implementation, and sendsmigrate(migration). Its read-only calls now run withoutETH_KEYSTORE, which cast otherwise tries to unlock, failing silently without a tty;josuke acceptrecords the routes.make install-evmbuilds the pinned evm and installs it beside forge.tools/README.mddocuments the sequence.Compatibility
FilecoinWarmStorageServiceABI is unchanged. TheFWSSStoragefield is_viewContractAddress, so modules don't all export a getter; the monolith andFWSSViewContractModuledeclareviewContractAddress(). Module and transition ABIs aren't published; the merged proxy ABI comes separately, before this stack reaches main.usdfcTokenAddress()and the other former immutable getters stay unrouted until their business modules land, and StateViewgetPriceList()waits for them too.FWSSMigrateModuleinheritsFWSSStorageand uses its legacynextUpgradefield. The transition adds no storage.josuke deploycan't generate the migration yet. Josuke also runs everyjosuke.jsonentry against the connected chain with its top-leveladdress.Testing
The transition tests run against the deployed mainnet v1.4.0 proxy and implementation bytecode, not a rebuild. The scripts ran on an anvil chain with the v1.4.0 bytecode etched at the mainnet addresses and a hand-written josuke ledger: dispatcher and transition deploy, dry run, announcement, and the Safe calldata from the execute script. A real
josuke deployagainst a v1.4.0 proxy waits on josuke#3.From
service_contracts/:make force-gen && make update-abi,forge fmt --check,forge lint --deny notes --quiet,make build test(929 passed),GITHUB_BASE_REF=refactor/fwss-modular-dispatch make check-layout,make contract-size-check,bash tools/check_deployments_checksums.sh deployments.json.