ABI decoder by chriseth · Pull Request #2863 · argotorg/solidity

chriseth · 2017-09-01T17:52:49Z

always check that offset + min size <= size, if possible do it efficiently (i.e. group statically sized types) - reconfirm this, currently we only check offset < end and not offset + size < end
implement struct decoding
manual coverage testing
clarify if we need to call "interfaceType()" for the decoder types.

axic · 2017-09-01T17:54:28Z

libsolidity/codegen/ContractCompiler.cpp

 	// We do not check the calldata size, everything is zero-padded

+	TODO: What about external functions?
+	if (m_context.experimentalFeatureActive(ExperimentalFeature::ABIEncoderV2))


ABIDecoderV2?

Perhaps we should have named it differently. Do you want to have a second experimental feature name?

It is a different feature, isn't? Or do you think they would mature the same rate? I mean the question is: would you consider to promote one without the other out of experimental status or would prefer to only promote them together.

The main feature is being able to call a function on a different contract and pass structs and nested arrays. You always need both the encoder and the decoder.

I also think that the decoder should be easier to write, because you do not have to deal with storage.

axic · 2017-09-01T17:54:55Z

libsolidity/codegen/CompilerUtils.cpp

+	auto ret = m_context.pushNewTag();
+	m_context << Instruction::SWAP1;
+
+	string decoderName = m_context.abiFunctions().tupleDecoder(_givenTypes, _targetTypes, _encodeAsLibraryTypes);


tupleDecoder is actually missing

This was just a quick 'save to github in case anything happens over the weekend'. It practically contains no code ;)

chriseth · 2017-09-12T14:57:54Z

Latest version should work for all value types - but does not yet have any tests.

axic · 2017-09-19T11:07:28Z

@chriseth can you rebase on top of #2924 ?

axic · 2017-09-19T11:08:23Z

Changelog.md

 * Optimizer: Add new optimization step to remove unused ``JUMPDEST``s.
 * Type Checker: Warn on using literals as tight packing parameters in ``keccak256``, ``sha3``, ``sha256`` and ``ripemd160``.
+ * Code Generator: New ABI decoder which supports structs and arbitrarily nested
+   arrays and checks input size (activate using ``pragma experimental ABIEncoderV2;``.


Missing closing parentheses.

axic · 2017-09-21T13:17:06Z

libsolidity/ast/Types.h

 	{
 		return location() == DataLocation::Storage ? std::make_shared<IntegerType>(256) : shared_from_this();
 	}
+	virtual TypePointer decodingType() const override;


encodingType above seems to be the exact same? Also if two functions must exists it is probably better placing them next to each other, either header or code.

They are different for some types.

Ah right, but removed it from struct because the default is to use the encoding type.

axic · 2017-09-21T13:20:53Z

libsolidity/codegen/ABIFunctions.cpp

 		case Type::Category::Struct:
-			solAssert(false, "Struct cleanup requested.");
+			solAssert(_type.dataStoredIn(DataLocation::Storage), "Cleanup requested for non-storage reference type.");
+			templ("body", "cleaned := value");


Where is the actual cleanup?

There is no cleanup to be performed for a storage pointer. All 2**256 values are valid.

So this part of the change is not restricted to this PR?

the encoder does not call cleanup on structs - the control flow branches off earlier.

Or actually, better explanation: The encoder has a "source type" and a "target type", while the decoder only has a single type. For structs, the target type is already an integer and not a struct, and thus the cleanup will be called on the integer.

axic · 2017-09-21T13:21:26Z

libsolidity/codegen/ABIFunctions.cpp

+	for (auto const& t: _types)
+		functionName += t->identifier();
+	if (_fromMemory)
+		functionName += "_fromMemory";


Hm, wondering if we should commit to camelcase or underscores, this one mixes the two.

underscores are used to separate components.

chriseth · 2017-09-25T17:39:27Z

Rebased, fixed some stuff, did manual coverage tests and ran against full EndToEnd tests.

The only thing left here is the decoder for structs.

chriseth · 2017-10-04T12:59:47Z

@axic this is ready for review.

axic · 2017-10-05T11:26:19Z

Needs rebasing since the other ABI PRs were merged.

axic · 2017-10-05T11:26:58Z

libsolidity/codegen/CompilerUtils.h

+	/// Can allocate memory.
+	/// Stack pre: <source_offset>
+	/// Stack post: <value0> <value1> ... <valuen>
+	void abiDecode(TypePointers const& _parameterTypes, bool _fromMemory = false);


For consistency I'd go with abiDecodeV2 for the moment.

axic · 2017-10-05T11:42:33Z

libsolidity/codegen/ContractCompiler.cpp

+		// Use the new JULIA-based decoding function
+		auto stackHeightBefore = m_context.stackHeight();
+		CompilerUtils utils(m_context);
+		utils.abiDecode(_typeParameters, _fromMemory);


Rest of the code uses CompilerUtils(m_context).

axic · 2017-10-05T12:31:12Z

libsolidity/codegen/ABIFunctions.h

+	/// into memory. If @a _fromMemory is true, decodes from memory instead of
+	/// from calldata.
+	/// Can allocate memory.
+	/// Inputs: <source_offset> <source_end> (layout reversed on stack)


These are the inputs if _fromMemory is true, otherwise no inputs or is it an offset within the calldata?

these are either pointers to calldata or to memory.

Also, are those both absolute pointers or is source_end actually source_len?

Both are absolute pointers.

axic · 2017-10-05T12:32:11Z

libsolidity/codegen/ABIFunctions.h

+	/// Outputs: <value0> <value1> ... <valuen>
+	/// The values represent stack slots. If a type occupies more or less than one
+	/// stack slot, it takes exactly that number of values.
+	std::string tupleDecoder(TypePointers const& _types, bool _fromMemory = false);


So it returns the function name which has a signature of <name>(source_offset, source_end) -> <v0>, <v1>, ..., <vn>. is that correct?

Cool, I was going to try it in the IR pull request and that API should work well.

axic · 2017-10-13T10:01:47Z

libsolidity/codegen/ABIFunctions.h

 		bool _fromStack
 	);

+	/// @returns the name of the ABI decodinf function for the given type


axic · 2017-10-13T10:02:19Z

libsolidity/codegen/CompilerContext.cpp


 	ErrorList errors;
 	ErrorReporter errorReporter(errors);
+//	cout << _assembly << endl;


Stray comment

axic · 2017-10-13T10:10:41Z

libsolidity/codegen/ContractCompiler.cpp

 	}
 	m_context.appendMissingLowLevelFunctions();
 	string abiFunctions = m_context.abiFunctions().requestedFunctions();
+//	cout << abiFunctions << endl;


Stray comment

axic · 2017-10-13T10:12:29Z

test/libsolidity/ABIDecoderTests.cpp

+
+BOOST_FIXTURE_TEST_SUITE(ABIDecoderTest, SolidityExecutionFramework)
+
+BOOST_AUTO_TEST_CASE(BOTH_ENCODERS_macro)


Mixed case test case name.

chriseth · 2017-11-16T13:03:28Z

Closes #3049

chriseth · 2017-11-16T13:03:41Z

@axic this is ready to be merged from my side.

axic · 2017-11-22T04:00:49Z

Can some of the commits be squashed? Many just say "decoder", "decoder fixes", "decoder tests" :)

axic · 2017-11-22T04:01:12Z

Also need to be rebased and changelog updated.

chriseth · 2017-11-22T13:06:48Z

Rebased and changed changelog.

axic · 2017-11-22T13:15:04Z

test/libsolidity/ABIEncoderTests.cpp

 	BOTH_ENCODERS(
 		compileAndRun(sourceCode);
-		callContractFunction("f(uint256)");
+		callContractFunction("f(uint256)", u256(0));


Just remind me: does the newer decoder changes semantics and requires the minimum length of the encoding present as calldata?

Do we have a specific test for short data with the new decoder?

Yes, the new decoder does not tolerate short data and there are multiple tests.

axic · 2017-11-22T13:17:35Z

test/libsolidity/ABIDecoderTests.cpp

+		compileAndRun(sourceCode);
+		ABI_CHECK(callContractFunction("f(uint256,uint256)", 1, 2), encodeArgs(1));
+		ABI_CHECK(callContractFunctionNoEncoding("f(uint256,uint256)", bytes(64, 0)), encodeArgs(0));
+		ABI_CHECK(callContractFunctionNoEncoding("f(uint256,uint256)", bytes(63, 0)), encodeArgs());


This tests the new encoder to abort on short input. Should this case have the same case for the old encoder allowing it?

I can add that if you want.

chriseth · 2017-11-23T18:56:21Z

Added short input tests for all types the old decoder can handle.

axic · 2017-11-27T14:37:36Z

libsolidity/codegen/ABIFunctions.cpp

+				memPtr := mload(<freeMemoryPointer>)
+				let newFreePtr := add(memPtr, size)
+				// protect against overflow
+				switch or(gt(newFreePtr, 0xffffffffffffffff), lt(newFreePtr, memPtr)) case 1 { revert(0, 0) }


Could be changed to if.

I will check all switches later, I think we should merge this pr without that change.

Never mind, the only really safe ones without review are the those checking for case 0.

chriseth · 2017-11-28T10:19:35Z

Can this be merged?

chriseth added the in progress label Sep 1, 2017

axic reviewed Sep 1, 2017

View reviewed changes

chriseth force-pushed the structDecoder branch from 9c759b9 to 053c998 Compare September 12, 2017 14:57

axic reviewed Sep 19, 2017

View reviewed changes

chriseth force-pushed the structDecoder branch from db4662d to 0a4cfc6 Compare September 20, 2017 17:31

axic reviewed Sep 21, 2017

View reviewed changes

chriseth force-pushed the structDecoder branch 3 times, most recently from e11e681 to 4992c5f Compare September 25, 2017 17:38

chriseth force-pushed the structDecoder branch from 4992c5f to 5cfde8d Compare September 26, 2017 12:56

chriseth changed the title ~~[WIP] ABI decoder~~ ABI decoder Oct 4, 2017

axic reviewed Oct 5, 2017

View reviewed changes

chriseth force-pushed the structDecoder branch from 0edf658 to 27e7b46 Compare October 5, 2017 16:00

chriseth mentioned this pull request Oct 9, 2017

No support for int[][] (string[], etc.) function parameters. #40

Closed

chriseth force-pushed the structDecoder branch from 27e7b46 to fe1230a Compare October 11, 2017 13:45

axic reviewed Oct 13, 2017

View reviewed changes

chriseth force-pushed the structDecoder branch from 6072d29 to 019b816 Compare November 16, 2017 13:03

chriseth force-pushed the structDecoder branch from 019b816 to cc3b651 Compare November 16, 2017 15:54

axic mentioned this pull request Nov 22, 2017

Enable struct abi example with experimental pragma #3228

Merged

chriseth force-pushed the structDecoder branch from cc3b651 to 80bce92 Compare November 22, 2017 13:06

axic reviewed Nov 22, 2017

View reviewed changes

axic added the nextrelease label Nov 22, 2017

chriseth force-pushed the structDecoder branch from 80bce92 to b3f2daf Compare November 23, 2017 18:56

chriseth force-pushed the structDecoder branch from b3f2daf to d74599d Compare November 27, 2017 14:33

axic reviewed Nov 27, 2017

View reviewed changes

axic approved these changes Nov 27, 2017

View reviewed changes

chriseth added 4 commits November 29, 2017 17:08

ABI decoder.

bdc1ff8

Decoder tests.

98c3810

Cleanup and overflow checks for data pointers.

5a3dbb0

Also test short input for old decoder.

9d8e3ff

chriseth force-pushed the structDecoder branch from d74599d to 9d8e3ff Compare November 29, 2017 16:08

axic merged commit 0759147 into develop Nov 29, 2017

axic deleted the structDecoder branch November 29, 2017 20:02

axic removed in progress labels Nov 29, 2017

This was referenced Apr 3, 2018

Support returning structs from functions BANKEX/web3swift#33

Closed

Struct encoder/decoder: Using a struct in a public function with ABIEncoderV2 BANKEX/web3swift#32

Closed

n0izn0iz mentioned this pull request Aug 7, 2024

[chain] ABI-like support for Realm / Package requests / responses gnolang/gno#1842

Open


		BOOST_FIXTURE_TEST_SUITE(ABIDecoderTest, SolidityExecutionFramework)

		BOOST_AUTO_TEST_CASE(BOTH_ENCODERS_macro)

Conversation

chriseth commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Sep 12, 2017

Uh oh!

axic commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Sep 25, 2017

Uh oh!

chriseth commented Oct 4, 2017

Uh oh!

axic commented Oct 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Nov 16, 2017

Uh oh!

chriseth commented Sep 1, 2017 •

edited

Loading

chriseth Sep 1, 2017 •

edited

Loading

axic Oct 5, 2017 •

edited

Loading