fix(world): support expectRevert and unusual nameless arguments in system libraries

[dk1a]

4 issues:

1. Using vm.expectRevert before a system library call always fails with a confusing EvmError: Revert, because the call is unexpectedly successful, but with an empty result, which breaks abi.decode(result, (...))
2. When an address payable parameter doesn't have a name, address is parsed as its type, and payable as its name
3. When a dynamic type (e.g. bytes calldata) parameter doesn't have a name, / calldata / in formatParams doesn't capture it because there is no whitespace at the end
4. When a return parameter has calldata location, it doesn't get replaced with memory, unlike arguments

2-4 are obviously related and fixed by the changes to formatParams, and formatting returnParameters too

1 is much less related, but the fix for it is - after I conditionally skip return if result.length == 0, this causes the solidity warning Unnamed return variable can remain unassigned. Add an explicit return with value to all non-reverting code paths or name the variable. solidity(6321).
And using the same formatParameters for returnParameters, including aux names, avoids the warning by naming the variables

I thought it'd be more convenient to review this way, but I can separate the expectRevert stuff if you don't like it

[changeset-bot]

🦋 Changeset detected

Latest commit: 94354ea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages

Name	Type
@latticexyz/store	Patch
@latticexyz/world-module-metadata	Patch
@latticexyz/world	Patch
@latticexyz/cli	Patch
@latticexyz/dev-tools	Patch
@latticexyz/entrykit	Patch
@latticexyz/explorer	Patch
@latticexyz/react	Patch
@latticexyz/stash	Patch
@latticexyz/store-indexer	Patch
@latticexyz/store-sync	Patch
@latticexyz/world-consumer	Patch
@latticexyz/world-module-callwithsignature	Patch
@latticexyz/world-module-erc20	Patch
@latticexyz/world-modules	Patch
@latticexyz/abi-ts	Patch
@latticexyz/block-logs-stream	Patch
@latticexyz/common	Patch
@latticexyz/config	Patch
create-mud	Patch
@latticexyz/faucet	Patch
@latticexyz/gas-report	Patch
@latticexyz/paymaster	Patch
@latticexyz/protocol-parser	Patch
@latticexyz/recs	Patch
@latticexyz/schema-type	Patch
solhint-config-mud	Patch
solhint-plugin-mud	Patch
@latticexyz/utils	Patch
vite-plugin-mud	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vdrg]

love these 🔥 thank you boss

[vdrg]

getting back to this now @dk1a , I was thinking that maybe instead of skipping the decoding when length is 0, we could use a lower level call (same as we do for view functions), that way the world call would not revert and we would have the control to make it revert with something like revertWithBytes directly from the library

[dk1a]

I was thinking that maybe instead of skipping the decoding when length is 0, we could use a lower level call (same as we do for view functions)

The view functions also get a length == 0 check actually (see SystemALib for example), after revertWithBytes which is based on success result
Due to how forge handles expectRevert there is no way to tell the difference between a success and an expected revert without checking the data bytes. Natively called functions are an exception because I guess forge handles them in a special way internally (not sure, but I imagine they return 0 bytes same as what I do here)