latest aggregation contracts

[se3000]

Cherry-picked onto the latest master. Still has a fair amount of outstanding work to do, but probably better to get it into master sooner than have to deal with large rebases/cherry-picks.

Adds limited support for aggregating oracle answers.

[coventry]

Other than these comments, LGTM.

[coventry]

Using SafeMath#add means that the oracles don't get paid if the total of their responses overflow. A malicious consumer could use this to elicit unrewarded work from oracles.

[coventry]

This is mixing the result-aggregation logic with the chainlink-business logic of paying the oracles. We will almost certainly want to make the aggregation logic pluggable, because it's very application-sensitive. Might want to make this two separate loops, even though they're iterating over the same set.

[thodges-gh]

Would checking if the value is less than the max value for uint248 before running aggregation work for us?

For example:

require(uint256(_data) < MAX_UINT248, "Number too big");

Unfortunately we're at the stack limit in this function as well, so we wouldn't be able to implement this check until aggregation is pulled out to its own function.

[coventry]

If the above calculation fails, this removal of the callback will not execute. Might matter, once storage is rented rather than bought.

[thodges-gh]

SafeMath's .div() operation has no assertion:

  /**
  * @dev Integer division of two numbers, truncating the quotient.
  */
  function div(uint256 _a, uint256 _b) internal pure returns (uint256) {
    // assert(_b > 0); // Solidity automatically throws when dividing by 0
    // uint256 c = _a / _b;
    // assert(_a == _b * c + _a % _b); // There is no case in which this doesn't hold
    return _a / _b;
  }

[j16r]

SaveMath won't, but it looks like solidity will?

[thodges-gh]

In what case would aggregation run with 0 responses, which is where we would be dividing by 0?

[j16r]

Not sure, just pointing out that even though SaveMath has no assertion, div can still fail.

[thodges-gh]

So basically how I see it is the fix for this is to use SafeMath for the addition operation on line 235, incrementing the responseCount. Just checking, are we good with that?

[thodges-gh]

Following up myself on this and I think we should make this change. The responseCount variable now is a uint8 (I had assumed it was already uint256), and we're not checking that some max number of oracles is being sent in for the initiateServiceAgreement function. So, assuming we didn't hit the gas limit for a block, we could overflow with 256 oracles, which SafeMath would guard against. This would also require changing responseCount in the Callback struct to uint256.

[dimroc]

Optional
Should this be public? Even if this doesn't have public setters, do we want people to be able to read its value?

I'm focusing on the reality that some are private, and some are public, and the inconsistency probably stems from testability. Let's make sure that's ok.

[thodges-gh]

Anyone would be able to read the storage of a deployed contract anyway in order to see "private" values.

[dimroc]

Optional
You add to now twice. Perhaps there's gas savings if you DRY it up.

uint64 memory expiry = node.add(EXPIRY_INTERVAL)

[thodges-gh]

We've hit the stack limit in this function. Introducing a new variable to store the expiry time won't compile.

[dimroc]

Optional
Suggestion for above, below, and this whole method in general. If there are no gas implications, would it be better to invoke methods that document their purpose in the name?

if responseIncomplete(oracles) {
  return true
}

fullfillWithAverage(...)

If there are gas considerations or the indirection reduces audit-ability, ignore.

[thodges-gh]

You are correct, we're eventually going to call the averaging function specified by the requester in the service agreement. For example:

return address(this).call(_aggFuncId, _requestId, _callbackAddress, _callbackFunctionId);

However, I think this story just covers the most basic form of aggregation.

[se3000]

Broke out a couple small helper methods. Possibly premature, given that this area is going to be refactored quite a bit, but it should also help comprehension when we get back to it.