Add abi.encodeError #14974

Amxx · 2024-04-02T21:08:05Z

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.
tests
review

github-actions · 2024-04-02T21:08:17Z

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.

nikola-matic

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

nikola-matic · 2024-04-03T09:54:46Z

test/libsolidity/syntaxTests/abiEncoder/abi_encodeError_free_func.sol

+    abi.encodeError(g, (1));
+}
+// ----
+// TypeError 3510: (56-57): Expected an error type. Cannot use functions for abi.encodeError.


Needs another case for library functions.

r0qs · 2024-04-03T10:02:31Z

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.

Amxx · 2024-04-03T11:50:38Z

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 · 2024-04-03T14:46:29Z

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?

cameel · 2024-04-03T16:33:31Z

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.

matheusaaguiar

This needs documentation entries in https://docs.soliditylang.org/en/v0.8.23/cheatsheet.html and https://docs.soliditylang.org/en/v0.8.23/units-and-global-variables.html#abi-encoding-and-decoding-functions (at least, there may be other places, but those are the main ones I guess).
I think the implementation is good, just left some minor suggestions. However, I think we could avoid the duplicated code with some ifs and ternary conditions and have it all in one typeCheckABIEncodeCallFunctionOrError (or something like that), but I don't consider that should impede this PR from being merged.

libsolidity/analysis/TypeChecker.cpp

Co-authored-by: matheusaaguiar <95899911+matheusaaguiar@users.noreply.github.com>

into a single typeCheckABIEncodeCallFunctionOrError

Amxx · 2024-05-03T11:16:12Z

@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)

matheusaaguiar · 2024-05-03T18:38:24Z

docs/cheatsheet.rst

@@ -26,6 +26,8 @@ ABI Encoding and Decoding Functions
  tuple. Performs a full type-check, ensuring the types match the function signature. Result equals ``abi.encodeWithSelector(functionPointer.selector, (...))``
 - ``abi.encodeWithSignature(string memory signature, ...) returns (bytes memory)``: Equivalent
  to ``abi.encodeWithSelector(bytes4(keccak256(bytes(signature))), ...)``
+- ``abi.encodeError(error errorPointer, (...)) returns (bytes memory)``: ABI-encodes the revert data for ``errorPointer`` with the arguments found in the
+  tuple. Performs a full type-check, ensuring the types match the error definition. Results equals ``abi.encodeWithSelector(errorPointer.selector, (...))``


Suggested change

tuple. Performs a full type-check, ensuring the types match the error definition. Results equals ``abi.encodeWithSelector(errorPointer.selector, (...))``

tuple. Performs a full type-check, ensuring the types match the error definition. Results equals ``abi.encodeWithSelector(errorPointer.selector, ...)``

This a minor imprecision that was fixed recently in the previous entry for abi.encodeCall :)
~~I will already commit this suggestion via github interface;~~
Actually, there were another instances which I already fixed via a commit.

matheusaaguiar

I just have one nitpick left, but overall looks good to me.
Also, I forgot to mention that we use rebase rather than merge and this needs the former.

matheusaaguiar · 2024-05-03T19:27:04Z

libsolidity/analysis/TypeChecker.cpp

+				m_errorReporter.typeError(
+					5512_error,
+					arguments.front()->location(),
+					"Expected first argument to be a custom error, not \"" +


I was trying to think of some test cases and when I try something like:

error E(uint a, uint b); contract C { function g() public view returns (bytes memory) { bytes memory b1 = abi.encodeError(E(1, 2), (1, 2)); return b1; } }

The following error message is generated:
Expected first argument to be a custom error, not "error".
Which is a bit weird. Maybe we should be more precise and change to [...] custom error name [...] ?
I think we should have a test like this anyway.

ekpyron · 2024-05-08T16:04:07Z

Pending the discussion in #14287 (comment), 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.

Add abi.encodeError

c62d1b7

github-actions bot added the external contribution ⭐ label Apr 2, 2024

Amxx added 2 commits April 2, 2024 23:21

deduplicate errors

eeff11d

testing

800b934

nikola-matic reviewed Apr 3, 2024

View reviewed changes

Amxx added 3 commits April 3, 2024 13:56

fix test file name

2a7250c

add semanticTests - viaIr not working

48e6e89

fix IR generator

513ea59

Amxx added 2 commits April 3, 2024 21:11

simplify

a33001b

add changelog entry

f822c13

Amxx requested a review from nikola-matic April 3, 2024 19:35

matheusaaguiar reviewed May 2, 2024

View reviewed changes

libsolidity/analysis/TypeChecker.cpp Outdated Show resolved Hide resolved

libsolidity/analysis/TypeChecker.cpp Outdated Show resolved Hide resolved

libsolidity/analysis/TypeChecker.cpp Outdated Show resolved Hide resolved

Amxx and others added 6 commits May 2, 2024 09:50

Merge branch 'develop' into feature/abi.encodeError

9fd94c9

Apply suggestions from code review

67f3b1d

Co-authored-by: matheusaaguiar <95899911+matheusaaguiar@users.noreply.github.com>

documentation

1960a3f

fix tests after merge

66f0833

merge typeCheckABIEncodeCallFunction and typeCheckABIEncodeErrorFunction

90c6943

into a single typeCheckABIEncodeCallFunctionOrError

re-apply suggestion for PR review

9c2845a

matheusaaguiar reviewed May 3, 2024

View reviewed changes

fixup! correct minor imprecision

e53f81d

matheusaaguiar approved these changes May 3, 2024

View reviewed changes

ekpyron closed this May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add abi.encodeError #14974

Add abi.encodeError #14974

Amxx commented Apr 2, 2024 •

edited

github-actions bot commented Apr 2, 2024

nikola-matic left a comment

nikola-matic Apr 3, 2024

r0qs commented Apr 3, 2024

Amxx commented Apr 3, 2024 •

edited

Amxx commented Apr 3, 2024

cameel commented Apr 3, 2024 •

edited

matheusaaguiar left a comment

Amxx commented May 3, 2024

matheusaaguiar May 3, 2024 •

edited

matheusaaguiar left a comment •

edited

matheusaaguiar May 3, 2024 •

edited

ekpyron commented May 8, 2024

	tuple. Performs a full type-check, ensuring the types match the error definition. Results equals ``abi.encodeWithSelector(errorPointer.selector, (...))``
	tuple. Performs a full type-check, ensuring the types match the error definition. Results equals ``abi.encodeWithSelector(errorPointer.selector, ...)``

Add abi.encodeError #14974

Add abi.encodeError #14974

Conversation

Amxx commented Apr 2, 2024 • edited

github-actions bot commented Apr 2, 2024

nikola-matic left a comment

Choose a reason for hiding this comment

nikola-matic Apr 3, 2024

Choose a reason for hiding this comment

r0qs commented Apr 3, 2024

Amxx commented Apr 3, 2024 • edited

Amxx commented Apr 3, 2024

cameel commented Apr 3, 2024 • edited

matheusaaguiar left a comment

Choose a reason for hiding this comment

Amxx commented May 3, 2024

matheusaaguiar May 3, 2024 • edited

Choose a reason for hiding this comment

matheusaaguiar left a comment • edited

Choose a reason for hiding this comment

matheusaaguiar May 3, 2024 • edited

Choose a reason for hiding this comment

ekpyron commented May 8, 2024

Amxx commented Apr 2, 2024 •

edited

Amxx commented Apr 3, 2024 •

edited

cameel commented Apr 3, 2024 •

edited

matheusaaguiar May 3, 2024 •

edited

matheusaaguiar left a comment •

edited

matheusaaguiar May 3, 2024 •

edited