Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[isoltest] Add support for events. #10728

Closed
wants to merge 7 commits into from
Closed

[isoltest] Add support for events. #10728

wants to merge 7 commits into from

Conversation

aarlt
Copy link
Member

@aarlt aarlt commented Jan 8, 2021

Depends on #10867.

Fixes #6902.

  • support for builtin functions.

    • basic builtin functions.
    • basic support to enable usage of builtin functions within expectations.
  • implementation of all event specific builtin functions.

  • add implicit check for events.

  • support automatic expectation generation (easily readable), if events where found but not checked for.

  • add useful builtins that can be used within expectations. (e.g. contract.address, utils.keccak256) - maybe this should be done in another PR

  • add log specific expectation builtins e.g. logs.sender.

  • move event specific tests from SolidityEndToEndTests to semantic tests.

@aarlt aarlt force-pushed the isoltest-events branch 2 times, most recently from bf5bace to 581afdd Compare January 8, 2021 03:59
@chriseth
Copy link
Contributor

chriseth commented Jan 8, 2021

How do you imagine such a test file to look in the end? I think it is not too important to support logs that are not produced by events.

@aarlt
Copy link
Member Author

aarlt commented Jan 8, 2021

How do you imagine such a test file to look in the end? I think it is not too important to support logs that are not produced by events.

Given this SolidityEndToEndTest:

BOOST_AUTO_TEST_CASE(event_emit)
{
	char const* sourceCode = R"(
contract ClientReceipt {
	event Deposit(address indexed _from, bytes32 indexed _id, uint _value);
	function deposit(bytes32 _id) public payable {
		emit Deposit(msg.sender, _id, msg.value);
	}
}
	)";
	ALSO_VIA_YUL(
		DISABLE_EWASM_TESTRUN()

		compileAndRun(sourceCode);
		u256 value(18);
		u256 id(0x1234);
		for (bool manually: {true, false})
		{
			callContractFunctionWithValue("deposit(bytes32,bool)", value, id, manually);
			BOOST_REQUIRE_EQUAL(numLogs(), 1);
			BOOST_CHECK_EQUAL(logAddress(0), m_contractAddress);
			BOOST_CHECK_EQUAL(h256(logData(0)), h256(u256(value)));
			BOOST_REQUIRE_EQUAL(numLogTopics(0), 3);
			BOOST_CHECK_EQUAL(logTopic(0, 0), util::keccak256(string("Deposit(address,bytes32,uint256)")));
			BOOST_CHECK_EQUAL(logTopic(0, 1), h256(m_sender, h256::AlignRight));
			BOOST_CHECK_EQUAL(logTopic(0, 2), h256(id));
		}
	)
}

The resulting semantic test may look like this:

contract ClientReceipt {
    event Deposit(address indexed _from, bytes32 indexed _id, uint _value);
    function deposit(bytes32 _id) public payable {
        emit Deposit(msg.sender, _id, msg.value);
    }
}
// ====
// compileViaYul: also
// ----
// deposit(bytes32), 18 wei: 0x1234 ->
// logs.numLogs() -> 1
// logs.logAddress(uint256): 0 -> contract.address() 
// logs.logData(uint256): 0 -> 18
// logs.numLogTopics(uint256): 0 -> 3
// logs.logTopic(uint256,uint256): 0, 0 -> utils.keccak256("Deposit(address,bytes32,uint256)")
// logs.logTopic(uint256,uint256): 0, 1 -> logs.sender()
// logs.logTopic(uint256,uint256): 0, 2 -> 0x1234

It would also be nice to support builtins (e.g. contract.address, utils.keccak256, logs.sender) for the expectations as shown here . But probably we don't need this at least right now.

Without builtins that can be used as expectations this test would look like this:

// deposit(bytes32), 18 wei: 0x1234 ->
// logs.numLogs() -> 1
// logs.logAddress(uint256): 0 -> 0x0fdd67305928fcac8d213d1e47bfa6165cd0b87b
// logs.logData(uint256): 0 -> 18
// logs.numLogTopics(uint256): 0 -> 3
// logs.logTopic(uint256,uint256): 0, 0 -> 0x19dacbf83c5de6658e14cbf7bcae5c15eca2eedecf1c66fbca928e4d351bea0f
// logs.logTopic(uint256,uint256): 0, 1 -> 0x1212121212121212121212121212120000000012
// logs.logTopic(uint256,uint256): 0, 2 -> 0x1234

@aarlt aarlt force-pushed the isoltest-events branch 2 times, most recently from 486b2c1 to 7ecf16c Compare January 9, 2021 00:11
@chriseth
Copy link
Contributor

Please make the test expectation more readable:

After each function that causes an event or a log, add one line per log, that looks as follows:

event: [<topic1>, <topic2>, ...] <decoded data> at <address>

With the following simplifications:

  • The "at " is removed if the address is the current contract
  • Try to see if the first topic is the hash of an event of the current contract. If you find a matching event, display its signature instead of the value - maybe inside <> or something.
  • You can try to decode the data according to the types of the event if you find a matching event - but that could also be a second step.

All emitted events always have to be mentioned. I think we should not make this a query to a builtin that can be there or cannot be there. Instead, the events belong to the function call.

@aarlt
Copy link
Member Author

aarlt commented Jan 13, 2021

I implemented this with the help of a builtin, but maybe it can be simplified more. I introduced a new builtin logs.expectEvent(uint256,string), where the first parameter is the index of the log, and the second parameter is the signature of the event. This builtin will basically just return the topics and the data.

If we have the following contract:

contract ClientReceipt {
    event D(address indexed _from, bytes32 indexed _id, uint _value);
    function deposit(bytes32 _id) public payable {
        emit D(msg.sender, _id, msg.value);
    }
}

The test looks right now like this:

// ====
// compileViaYul: false
// ----
// deposit(bytes32), 18 wei: 0x1234 ->
// logs.expectEvent(uint256,string): 0, "D(address,bytes32,uint256)" -> 0x1212121212121212121212121212120000000012, 0x1234, 18

As this is not an anonymous event, the builtin will check whether the provided event signature hash is equal to topic[0]. If this is not matching it will generate an error. In this case (non-anonymous event) topic[0] will be removed from the output of this builtin to simplify its usage. The builtin will at first output all topics, followed by the data. Here we see at first 0x1212121212121212121212121212120000000012, that is address, followed by 0x1234 that is the _id followed by value.

If we would change the event D to event D(address _from, bytes32 indexed _id, uint _value); The expectation would look like this: (address is not indexed and will become part of the data)

// logs.expectEvent(uint256,string): 0, "D3(address,bytes32,uint256)" -> 0x1234, 0x1212121212121212121212121212120000000012, 18

I implemented a simple tracking of event producers and it's consumer. The idea here is that only non-builtins can produce events, and only builtins - especially the logs builtins consume events. I know after each contract call what events where produced, and if after this call logs builtins where used and what logs where accessed (consumed). In the end I know per non-builtin call what events they produced and what events where consumed subsequent logs builtin calls.

This information is used to generate a failure during test execution, if not all produced events where consumed. Right now I only implemented the detection and visualisation that some events where not consumed. You can see this in the failing tests right now, e.g. here. The message will also show the event signature that was matching with the hash defined in topic[0].

You can try to decode the data according to the types of the event if you find a matching event - but that could also be a second step.

This is exactly what my plan is for tomorrow. Right now I only print the topics, and a big blob of data without interpreting the types.

The only missing thing is right now that I don't support your at <address>. I could imagine that probably the creator address is not always important, so we could introduce an additional builtin, e.g. logs.expectEventAt(uint256,string,address), where we can additionally check whether the log was created by that address.

@ekpyron
Copy link
Member

ekpyron commented Jan 13, 2021

Don't forget that we ideally not just want to have an error, if the logs in the expectations do not match the actually emitted events, but we'd also like to be able to have "(u)pdate" in an isoltest call automatically produce a complete list... Ideally without touching existing matching entries and only automatically complementing the list of existing event expectations with the auto-generated versions of the missing ones. But I guess you're just missing logic for decoding event data for that so far and wil add that as soon as it's there :-).

struct log_record
{
size_t index;
/// The address of the account which created the log.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these comments are redundant. What I would like to know the meaning of is index. Is it its position in the transaction or the call?

@@ -51,6 +51,25 @@ class ExecutionFramework
{

public:
struct log_record
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
struct log_record
struct LogRecord

@@ -51,6 +51,25 @@ class ExecutionFramework
{

public:
struct log_record
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same as in MockHost except for the index - do we need the duplication?

for (auto logIdx: m_touchedLogs[consumer])
// All logs that where touched by the consumer, will be marked as
// touched within the producer.
touchLog(producer->call(), logIdx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say let's rather check that the number is the same and they are equal individually.

@axic axic changed the base branch from develop to isoltest-builtins February 17, 2021 20:03
@aarlt aarlt force-pushed the isoltest-builtins branch 4 times, most recently from 8bf54d2 to febe7a3 Compare February 18, 2021 20:16
@aarlt aarlt force-pushed the isoltest-builtins branch 4 times, most recently from 30df8ae to 055c73c Compare February 19, 2021 01:28
@aarlt aarlt force-pushed the isoltest-builtins branch 6 times, most recently from 37bf423 to a9a6de2 Compare February 23, 2021 23:20
Comment on lines +17 to +20
// deposit(bytes32), 28 wei: 0x1234 -> 0x1c
// logs.expectEvent(uint256,string): 0, "A(address,bytes32,uint256)" -> 0x1212121212121212121212121212120000000012, 0x1234, 0x1c
// logs.expectEvent(uint256,string): 1, "" -> 0x1c, 0x1212121212121212121212121212120000000012, 0x1234
// logs.expectEvent(uint256,string): 2, "C(address,bytes32,uint256)" -> 0x1234, 0x1212121212121212121212121212120000000012, 0x1c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// deposit(bytes32), 28 wei: 0x1234 -> 0x1c
// logs.expectEvent(uint256,string): 0, "A(address,bytes32,uint256)" -> 0x1212121212121212121212121212120000000012, 0x1234, 0x1c
// logs.expectEvent(uint256,string): 1, "" -> 0x1c, 0x1212121212121212121212121212120000000012, 0x1234
// logs.expectEvent(uint256,string): 2, "C(address,bytes32,uint256)" -> 0x1234, 0x1212121212121212121212121212120000000012, 0x1c
// deposit(bytes32), 28 wei: 0x1234 -> 0x1c
// - emit A(address,bytes32,uint256) -> 0x1212121212121212121212121212120000000012, 0x1234, 0x1c
// - emit B(...) -> 0x1c, 0x1212121212121212121212121212120000000012, 0x1234

@aarlt aarlt force-pushed the isoltest-builtins branch 8 times, most recently from 7143c5b to 767514e Compare March 3, 2021 17:51
@aarlt aarlt force-pushed the isoltest-builtins branch 4 times, most recently from 59ddf67 to 9489582 Compare March 4, 2021 13:50
Base automatically changed from isoltest-builtins to develop March 4, 2021 14:42
@aarlt
Copy link
Member Author

aarlt commented Apr 30, 2021

Replaced by #11050.

@aarlt aarlt closed this Apr 30, 2021
@aarlt aarlt deleted the isoltest-events branch April 30, 2021 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[isoltest] Handle events in isoltest semantics tests.
3 participants