solidity icon indicating copy to clipboard operation
solidity copied to clipboard

Add abi.encodeError

Open Amxx opened this issue 1 year ago • 6 comments

Fixes #14287

TODO:

  • [ ] code deducpliation between typeCheckABIEncodeCallFunction and typeCheckABIEncodeErrorFunction ?
    • I'm not sure if there is a clean way of doing that. Considering there is already a lot of code duplication in the codebase, maybe that is ok.
  • [x] tests
  • [ ] review

Amxx avatar Apr 02 '24 21:04 Amxx

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

github-actions[bot] avatar Apr 02 '24 21:04 github-actions[bot]

This CI error: https://app.circleci.com/pipelines/github/ethereum/solidity/33577/workflows/8c3156a6-35f8-4aeb-b6e3-18f80f4cb6cf/jobs/1512784 is because the test file name is wrong, see: test/libsolidity/syntaxTests/specialFunctions/encodeCall_fail_args_internal_function_pointer_for_uint copy.sol. There is a _copy in the filename of this test https://github.com/ethereum/solidity/pull/14974/files#diff-41100eaa90ef551806d2098a1c4f80fe0f86d9587edbfb6f1161c6d543f1f92dR1.

r0qs avatar Apr 03 '24 10:04 r0qs

Needs some semantic tests as well. A changelog entry as well.

Yes, i'm still working on the testing part.

Edit: should be good now

Amxx avatar Apr 03 '24 11:04 Amxx

Apparently, in the semantic tests, the returned bytes memory are processed in chunks of 32 bytes. So when the buffer has length ≡ 4 mod 0x20, we need to pad the expected output with zeros (28 bytes worth of zero exactly)

Is that supposed to happen?

Amxx avatar Apr 03 '24 14:04 Amxx

Could be just another instance of #13989. The issue is about fixed bytes, but maybe it affects bytes too. I think that soltest is just not padding byte arrays correctly.

cameel avatar Apr 03 '24 16:04 cameel

@matheusaaguiar

  • I added some references in the files you mentionned
  • I fixed the merge conflicts and fixed what needed to be fixed
  • I also merged previous functions into typeCheckABIEncodeCallFunctionOrError (added the type object and some switches)

Amxx avatar May 03 '24 11:05 Amxx

Pending the discussion in https://github.com/ethereum/solidity/issues/14287#issuecomment-2096501517, we decided to close this PR for now. But feel free to jump in there (or join one of our calls) and make the case for this approach and we can potentially revisit and revive this PR! But as a general policy, we should discuss and confirm the approach before implementations, to be sure we're on the same page about it.

ekpyron avatar May 08 '24 16:05 ekpyron

@ekpyron This change to ERC-7751, if it was to happen (note, I'm a co-author of the ERC, so not unlikelly), would create a significant usecase of abi.encodeError.

Depending on the feedback this gets, I'd like to reconsider this feature.

Amxx avatar Sep 02 '24 09:09 Amxx