Add .selector member on function types by axic · Pull Request #2473 · argotorg/solidity

axic · 2017-06-27T11:12:38Z

chriseth

Strictly, this is about the first four bytes of the hash of the signature, not the signature itself. I know we have msg.sig, but I wonder whether it would make sense to call it identifier or id instead.

On the other hand, this could also be just confusing and sig is an accepted - even if wrong - name for this.

pirapira

Looks good to me.

pirapira · 2017-06-27T12:30:17Z

@chriseth identifier does not make sense because overloaded functions share the same identifier but have different hashes of function signatures.

chriseth

Please update the documentation. Also, there is a test failure:

Invalid error: "Internal compiler error (/home/travis/build/ethereum/solidity/libsolidity/ast/Types.cpp:2513): External signature of function needs declaration

"

scripts/../test/cmdlineTests.sh: line 80: 12148 Aborted                 (core dumped) "$REPO_ROOT"/build/test/solfuzzer --quiet < "$f"

Fuzzer failed on:

		contract C {

			function h() external {

			}

			function f() external returns (uint) {

				function () external g = this.h;

				return g.sig;

			}

		}

chriseth · 2017-06-27T12:48:16Z

The ABI specification calls this thing "Function Selector". Also msg.sig might sound like a cryptographic signature (especially in the context of a message).

Perhaps we should deprecate msg.sig and use the same word for both contexts.

funSel? functionSelector? selector?

federicobond · 2017-06-27T14:07:36Z

selector was clear for me, even before reading @chriseth's comment. I'm in favor of deprecating msg.sig and using selector in both contexts.

We can then talk about the function signature as the plain-text representation that is hashed to obtain the function selector.

axic · 2017-06-28T14:28:19Z

It is called as methodIdentifier and externalIdentifier in the compiler interface.

axic · 2017-06-28T14:48:35Z

This works:

                      function h() external {
                      }
                      function f() external returns (uint) {
                              var g = this.h;
                              return g.sig;
                      }

This doesn't:

                      function h() external {
                      }
                      function f() external returns (uint) {
                              function () external g = this.h;
                              return g.sig;
                      }

(Calling g() in both cases works properly and the proper signature is put in the assembly.)

The code fails here:

string FunctionType::externalSignature() const
{
        solAssert(m_declaration != nullptr, "External signature of function needs declaration");

axic · 2017-06-28T14:52:10Z

Probably sig on variables is not a good idea and var should be disabled, so have to check for hasDeclaration().

axic · 2017-07-01T10:44:27Z

Something is weird. On Linux it reports:

Running 1373 test cases...
Warning: This is a pre-release compiler version, please do not use it in production.
:5:12: Error: Return argument type uint32 is not implicitly convertible to expected type (type of first return variable) bytes4.
				return this.f.selector;
				       ^-------------^
/home/travis/build/ethereum/solidity/test/../test/libsolidity/SolidityExecutionFramework.h(68): error in "function_types_sig": Compiling contract failed
/home/travis/build/ethereum/solidity/test/libsolidity/../ExecutionFramework.h(72): fatal error in "function_types_sig": critical check !m_output.empty() failed
/home/travis/build/ethereum/solidity/test/libsolidity/SolidityNameAndTypeResolution.cpp(157): fatal error in "function_types_sig": critical check !!sourceAndError.second failed
*** 3 failures detected in test suite "SolidityTests"

But on Windows there is only one failure:

C:/projects/solidity/test/libsolidity/SolidityNameAndTypeResolution.cpp(157): fatal error: in "SolidityNameAndTypeResolution/function_types_sig": critical check !!sourceAndError.second has failed

What Linux reports is correct, but why is it missing on Windows?

axic · 2017-07-11T13:10:54Z

@chriseth any idea why the Windows vs. Linux tests differ?

chriseth · 2017-07-11T13:24:01Z

No, we have to investigate.

axic · 2017-07-11T13:49:40Z

libsolidity/ast/Types.cpp

+		if (m_kind == Kind::External && hasDeclaration())
+			members.push_back(MemberList::Member(
+				"selector",
+				make_shared<IntegerType>(32)


Should this be bytes4 or uint32?

axic · 2017-07-11T16:27:01Z

Difference is: the other compilation failure is part of the end-to-end tests, which is not run on Windows.

axic · 2017-07-11T17:06:50Z

@chriseth @pirapira @federicobond Added documentation changes so this should be ready.

axic · 2017-07-11T18:57:35Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+			}
+			function f() external returns (bytes4) {
+				var g = this.h;
+				return g.selector;


This one is not detected yet.

@chriseth if you know a quick way to solve this, please do

axic · 2017-08-23T12:01:54Z

@chriseth the reason we wanted to disable them on variables is most of them don't have a name so the signature cannot be established.

Tha question remains: should we allow it for var when it refers to an actual functions and as such has the name?

axic · 2017-08-28T22:12:51Z

@chriseth rebased (man, that was annoying) and I think this is as far as we can go without spending way too much time on it. It just checks if the declaration is available (e.g. has an external signature).

That means var assigning an actual external function works, but assigning anything via a function value type will not since that has no declaration (name).

chriseth · 2017-08-29T10:20:55Z

libsolidity/codegen/ExpressionCompiler.cpp

+		if (member == "selector")
+		{
+			FunctionType const& type = dynamic_cast<FunctionType const&>(*_memberAccess.expression().annotation().type);
+			utils().popStackElement(type);


Why not m_context << Instruction::SWAP1 << Instruction::POP;? (and then perhaps a shift left, not sure about that)

So the stack should have <address> <signature>.

axic · 2017-08-29T13:18:40Z

Test bound functions. Perhaps for function types the stack also contains the first argument either before or after the function type.

axic · 2017-09-12T15:03:06Z

@chriseth please review (but don't merge yet, will squash some commits)

chriseth · 2017-09-12T15:33:09Z

libsolidity/ast/Types.cpp

 	case Kind::BareDelegateCall:
 	{
 		MemberList::MemberMap members;
+		if (m_kind == Kind::External && hasDeclaration())


Didn't we say that the hasDeclaration() condition is not required?

Need to give it a try.

axic · 2017-09-13T09:09:47Z

test/libsolidity/SolidityEndToEndTest.cpp

+				return fun.selector;
+			}
+			function h() returns (bytes4) {
+				function () external returns (bytes4) fun = this.f;


This works now, but not sure we are doing the right thing, since the function signature of this function type can be different depending on what function was assigned to it.

Sure, but the correct type signature has to be there, since we also have to know it when we want to call the function. What are your worries?

Never mind.

axic · 2017-09-13T10:46:49Z

@chriseth should be ready now

axic added the in progress label Jun 27, 2017

axic force-pushed the functiontype-sig branch from fcfa374 to a226f4a Compare June 27, 2017 11:16

axic requested review from chriseth and pirapira June 27, 2017 11:25

chriseth reviewed Jun 27, 2017

View reviewed changes

pirapira approved these changes Jun 27, 2017

View reviewed changes

chriseth suggested changes Jun 27, 2017

View reviewed changes

axic force-pushed the functiontype-sig branch from a226f4a to 1fb0c3d Compare June 28, 2017 17:15

axic mentioned this pull request Jul 11, 2017

Add failing implicit conversion test #2557

Merged

axic commented Jul 11, 2017

View reviewed changes

axic force-pushed the functiontype-sig branch 5 times, most recently from 5703e70 to 2cd25bd Compare July 11, 2017 16:52

axic mentioned this pull request Jul 11, 2017

Meta-programming interface - ERC165 support #1447

Closed

axic force-pushed the functiontype-sig branch 2 times, most recently from 8ab9ca2 to 38e0b1a Compare July 11, 2017 18:56

axic commented Jul 11, 2017

View reviewed changes

axic changed the title ~~Add .sig member on function types~~ Add .selector member on function types Jul 27, 2017

axic added the nextrelease label Aug 11, 2017

axic force-pushed the functiontype-sig branch 3 times, most recently from e45d046 to 2ab217c Compare August 28, 2017 22:09

chriseth reviewed Aug 29, 2017

View reviewed changes

axic force-pushed the functiontype-sig branch from 2ab217c to cce5083 Compare September 12, 2017 15:02

chriseth reviewed Sep 12, 2017

View reviewed changes

axic force-pushed the functiontype-sig branch 3 times, most recently from aeee758 to 0fe2d04 Compare September 13, 2017 08:28

axic commented Sep 13, 2017

View reviewed changes

axic added 5 commits September 13, 2017 11:40

Add sig member on function type

58f7a27

Add tests for function type sigs

88946f9

Rename .sig to .selector on function types

fd1f8ab

Document function selectors

c70ebfd

Use hashing function in tests

8b166c3

axic force-pushed the functiontype-sig branch from 0fe2d04 to 8b166c3 Compare September 13, 2017 10:42

chriseth approved these changes Sep 13, 2017

View reviewed changes

axic merged commit 5c9dbd5 into develop Sep 13, 2017

axic deleted the functiontype-sig branch September 13, 2017 16:35

axic removed in progress labels Sep 13, 2017

axic mentioned this pull request Sep 13, 2017

Fix view/pure warnings on selector tests #2900

Merged

thedavidmeister mentioned this pull request Jun 26, 2023

full function signature string and hash, not just selector #14360

Closed

Conversation

axic commented Jun 27, 2017

Uh oh!

chriseth left a comment

Choose a reason for hiding this comment

Uh oh!

pirapira left a comment

Choose a reason for hiding this comment

Uh oh!

pirapira commented Jun 27, 2017

Uh oh!

chriseth left a comment

Choose a reason for hiding this comment

Uh oh!

chriseth commented Jun 27, 2017

Uh oh!

federicobond commented Jun 27, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

axic commented Jun 28, 2017

Uh oh!

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

Uh oh!

Uh oh!

axic commented Jul 11, 2017

Uh oh!

chriseth commented Jul 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

axic commented Jul 11, 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!

axic commented Aug 23, 2017

Uh oh!

axic commented Aug 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

axic commented Sep 12, 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!

axic commented Sep 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

axic commented Jun 28, 2017 •

edited

Loading

axic commented Jun 28, 2017 •

edited

Loading

axic commented Jul 1, 2017 •

edited

Loading

axic commented Jul 11, 2017 •

edited

Loading

axic commented Aug 29, 2017 •

edited

Loading