Uniswap / v2-core

🦄 🦄 Core smart contracts of Uniswap V2
https://uniswap.org/docs
GNU General Public License v3.0
2.97k stars 3.18k forks source link

Add dependency injected getTimestamp. #69

Closed MicahZoltu closed 4 years ago

MicahZoltu commented 4 years ago

When writing contracts that integrate with Uniswap, it would be a huge boon to be able to run Uniswap on a testnet with dependency injected getTimestamp function, rather than using the built-in timestamp.

contract TimestampProvider {
    function getTimestamp() external returns (uint256) {
        return block.timestamp;
    }
}
contract UniswapV2Exchange {
    TimestampProvider public timestampProvider;
    constructor(TimestampProvider _timestampProvider) {
        timestampProvider = _timestampProvider;
    }
    ...
}

Also a similar change to the factory so it can pass along the timestamp provider.

The idea here is that in a test environment, you would provide a TimestampProvider that had functions on it for setting the timestamp, rather than pulling it from the block. This makes it so people can test their Uniswap integrated code against real nodes (e.g., Geth, Parity, Nethermind) when their code depends on the passage of time (like oracles do).

Caveat: This will increase the runtime gas costs of Uniswap operations by an SLOAD + CALL + SLOAD (plus some miscellaneous stuff). I can appreciate the argument that it may not be worth it, but as someone who is currently doing a Uniswap oracle integration, it sure would be nice!

NoahZinsmeister commented 4 years ago

this seems excessive, isn't the far more elegant solution to simply mock the timestamp in your test environment?

MicahZoltu commented 4 years ago

That only works if you are testing solely against ganache (and its 880+ transitive dependencies totalling 200+ MB over the wire, including 11 vulnerabilities, and all of the headaches all that brings). If you want to test against Parity/Geth/Nethermind/TurboGeth, you need the ability to do contract-level mocking of timestamps.

NoahZinsmeister commented 4 years ago

this is out of scope for v2, but appreciate the feedback! will keep issues like this in mind for timestamp-related code in the future