๐ฆ ๐ฆ Core smart contracts of Uniswap V2
Home Page: https://uniswap.org/docs
License: GNU General Public License v3.0
In-depth documentation on Uniswap V2 is available at uniswap.org.
The built contract artifacts can be browsed via unpkg.com.
The following assumes the use of node@>=10.
yarn
yarn compile
yarn test
Let's have an argument for why we should do "reserves math" vs balance-checking.
The code currently calculates and stores both the accumulator for the price of both tokens. However, it seems that one can derive the price of one token from the price of the other (its just an inverse) so why waste the gas on both calculating doubly and storing doubly instead of just calculating and storing one? Is there some usage of the accumulator that only works when one of the two values is stored?
The current implementation of sqrt, copied here for convenience:
1. function sqrt(uint y) internal pure returns (uint z) {
2. if (y > 3) {
3. uint x = (y + 1) / 2;
4. z = y;
5. while (x < z) {
6. z = x;
7. x = (y / x + x) / 2;
8. }
9. } else if (y != 0) {
10. z = 1;
11. }
12. }
reverts given the argument uint(-1), as this sets x to zero, leading to a division by zero error at line 7.
An easy fix is to change L3 to uint x = y / 2 + 1;, which makes the function well defined for uint(-1) as well.
This require statement is pretty gnarly and difficult to grok. Consider breaking it up into multiple require statements like:
require(success, "UniswapV2: TRANSFER_FAILED");
require(data.length == 0 || abi.decode(data, (bool)), "UniswapV2: TRANSFER_FAILED");A comment here also wouldn't hurt to improve readability, something like:
If the function returned something, assume it was a success flag and check to make sure it is true.
Does it stay 0.3%?
Call with MetaMask?
Call with Augur?
I suspect that most people coming to look at this source code will not know what a UQ112 is. While it may be a known term of art in some small circle somewhere, I believe it is obscure to the majority of people thus is deserving of some commentary as to what it is. One option would be to just link to Wikipedia, though that may not stand the test of time. Another would be to just give a quick mention in a comment in the library code indicating that it is a 224-bit fixed point unsigned number scaled by 2^112. At least fixed point numbers have reasonable SEO, unlike Q (๐ mathematicians).
Following the pattern of "put as little code in as possible in the core contracts", you could remove the transfer calls and instead just burn the tokens and reduce the reserves. The caller would need to follow-up their calls with a call to skim, presumably done in the same transaction via the UniswapV2Router.
I'd like to use the chainId opcode in Istanbul to get the chainId for the approveMeta digest. Hopefully this can happen before the code freeze.
LPs vote on fees
volatility signals from oracle
This is a super nitpick, but when integrating with Uniswap v1, especially with auto-generated code, I always found the fact that the contract was named Exchange to be confusing. Naming it Uniswap is certainly a lot better, but it would be epsilon better IMO if it was named UniswapExchange instead.
In the factory, should we consider allowing the enumeration of all pairs for a given token?
The data structure could look like:
mapping (address => address[]) internal tokenToOtherTokens;...and could be updated like:
tokenToOtherTokens[token0].push(token1);
tokenToOtherTokens[token1].push(token0);Code tends to be more readable when large conditional blocks are extracted out into well named helper methods rather than inlining each branch. In this case, the function basically has two different code paths it diverges on and so it is a good opportunity to simplify the whole function down to just:
function move(address tokenIn, uint amountOut) external lock {
uint balance0; uint balance1; uint amountIn; uint fee;
address feeRecipient = IUniswapV2Factory(factory).feeRecipient();
if (tokenIn == token0) {
moveToken1(...);
} else {
moveToken0(...);
}
_update(balance0, balance1);
emit Move(msg.sender, tokenIn, amountIn, amountOut);
}It is worth noting that I believe you could actually just have a single function for bath branches that simply takes the token as a parameter. This would help deduplicate this code which is also very valuable for writing bug free code.
I think I agree with your current solution, but I wanted to bring this up just to make sure the conversation was had about using a constructor for the exchange rather than a separate initialize function. You can append the abi.encode(token0, token1) to the exchange deployment bytes in order to call the constructor with parameters. This way you wouldn't need to separately call an initialize function after deployment with CREATE2. This would also make it so you wouldn't need a separate salt variable for CREATE2, since the bytecode for each exchange would be different.
The downside here is that concatenating bytes requires assembly. The assembly required for simple bytes concatenation is pretty simple (just put the bytes in memory, followed by the constructor arguments, then change the length to include both), but this is assembly which is generally much more complicated and harder to audit.
IIUC, the only way to add liquidity at the moment is by transferring assets to the exchange contract and then calling make. This means that externally owned accounts (i.e., not-a-contract) cannot add liquidity without going through an intermediary contract. As much as I hate approvals, I believe that there is value in supporting EOAs adding liquidity and for ERC20 tokens approvals are the only way. For ERC777 tokens it is possible to add liquidity without approvals on one side, but you would still need approvals on the other side (unless you do #26).
The simple solution here is to issue a transfer-from from msg.sender to the exchange for both tokens when the user calls a method with a liquidity amount provided.
The other option is to use one-time contract accounts that the user can transfer assets to followed by calling the default function on the contract which would issue exchange.make followed by transferring the liquidity tokens to the user. I suspect this is the route you are planning to go, but I don't see any source code available for such a system. Does it exist in another repository perhaps?
if (_kLast == 0) return;
if (feeOn) {
...
} else {
...
}
I believe that this function exists to deal with some obscure edge cases (rounding perhaps)/attacks as it allows one to "reset" the exchange if it somehow ends up short on capital (e.g., the token contract burns Uniswap balances either on purpose or on accident). However, these situations are incredibly obscure and while I normally don't advocate for a lot of comments on code (the code should be easy to read without comments), in cases like this where the why is not obvious I believe there is significant value.
_getTokens is only referenced in one place, which is an external function. If the user already has the exchange (a necessary input for the external function) then they can simply get this information from the exchange directly without needing to go through the factory. From what I can tell, there is no need or benefit for duplicating the token information in this contract, so I recommend removing this variable, the associated getter, and the set that is happening on exchange creation.
In order for someone to verify Uniswap's code is legitimate, they need to run the same compiler with the same configuration and compare the output bytecode with the input bytecode. Without publishing the compiler-input.json that was used to generate the build artifacts, a user cannot easily validate that the build artifacts were in fact generated from the source code in this repository.
Recommend publishing the compiler-input.json as part of the build artifacts so people can verify the build artifacts themselves.
Hello. Is there any problem with uniswap today? Why we cant remove our tokens from liquidity pool?
Currently the price cumulative values are set as such:
price0CumulativeLast += uint(UQ112x112.encode(reserve0).qdiv(reserve1)) * blocksElapsed;
price1CumulativeLast += uint(UQ112x112.encode(reserve1).qdiv(reserve0)) * blocksElapsed;
This will actually set the prices of each asset to the price of the other asset.
Consider if token0 has 20 and token1 has 5.
This currently sets the price of token0 to 20 / 5 = 4 when it should be 1 / 4.
Consider bumping to latest version of Solidity before releasing. At the least, I recommend reviewing all fixed Solidity bugs between latest and this version to make sure that none of them affect you.
rm is not a valid command on Windows. Consider using an NPM module for this instead, or an inline node script with node -e '...'. If you bump the min version of Node supported to v12 (LTS) then you can replace the script line with:
node -e \"require('fs').rmdir('./build/', { recursive: true }, error => { if (error !== null) console.error(error) })\" && waffle waffle.json
(could be simplified if you drop the error logging)
The last parameter of this event is missing a name. Either remove the parameter if it is meaningless, or give it a name so integrators know what it is. The missing name also breaks my tooling, which makes me very sad.
A big selling point of ETH is the lack of need of approvals. Requiring all pairs be token-token means that everyone always has to do approvals when using Uniswap which increases user friction.
Do what Uniswap v1 did and always have ETH as a base. In order to support token-token pairs (which I still argue are a bad idea in general as it bifurcates liquidity, but it seems I have lost that battle), you could have two Uniswap exchange contracts one for token-token (like you have now) and one for token-eth. The factory can create either token-eth or token-token exchange based on inputs (or function being called) and the UI can trivially aggregate these two sets of contracts into a single UI. This would allow you to not have the code complexity that comes with trying to do branching inside of the exchanges based on denominating token vs ETH. You could reduce code duplication by using inheritance such that most of the business logic is inside a base contract (all the mathing and whatnot) while the derived contract would just have implementation code for the send function and receive function.
ERC777 supports approval-free transfers by way of wrappedEth.transfer(uniswap, amount, callbackData) where callbackData contains details of the attempted trade. The exchange would then execute the trade as normal, with the exception of not using msg.sender anywhere and instead use the from provided in the tokensReceived callback.
This is another place where commentary would be immensely valuable. The math being done here is sufficiently obscure to make it opaque to the reader what is actually happening here and, most importantly, why it is happening. This is obviously not a naive implementation, which is OK if there is a good reason for it to not be a naive implementation, but without any comments it just looks like an engineer being clever, and clever engineers are Badโข.
You can use the underlying symbols and encodePacked to generate a new symbol.
Simple guard clauses are easier to understand and audit than complex ones. Consider splitting all multi-condition guard clauses into multiple guard clauses.
The example here is just one particular example of this, but the pattern is followed throughout the codebase. This could be converted into:
require(msg.sender == factory, "UniswapV2: FORBIDDEN");
require(token0 == address(0), "UniswapV2: FORBIDDEN");
require(token1 == address(0), "UniswapV2: FORBIDDEN");It is worth noting that for the non-failing case, the gas costs are actually lower for separate guard clauses than they are for multi-condition guard clauses. Test case to prove this claim (since its results were surprising to me):
pragma solidity >0.5.0;
contract Test {
function test(uint256 inputA, uint256 inputB) public {
require(inputA != 0 && inputB != 0, "Error with long error message.");
}
function test2(uint256 inputA, uint256 inputB) public {
require(inputA != 0, "Error with long error message.");
require(inputB != 0, "Error with long error message.");
}
}Throw this into Remix and compile with either 0.5.15 or 0.6.0, then deploy to JS VM and execute each transaction with inputs of 1,2 and look at the transaction gas costs.
https://github.com/Uniswap/uniswap-v2-core/blob/d2ddbd95e8500cfd3669ab81b64f48956521ea6b/contracts/UniswapV2.sol#L68
https://github.com/Uniswap/uniswap-v2-core/blob/d2ddbd95e8500cfd3669ab81b64f48956521ea6b/contracts/UniswapV2.sol#L84
https://github.com/Uniswap/uniswap-v2-core/blob/d2ddbd95e8500cfd3669ab81b64f48956521ea6b/contracts/UniswapV2.sol#L99
https://github.com/Uniswap/uniswap-v2-core/blob/d2ddbd95e8500cfd3669ab81b64f48956521ea6b/contracts/UniswapV2.sol#L153
https://github.com/Uniswap/uniswap-v2-core/blob/d2ddbd95e8500cfd3669ab81b64f48956521ea6b/contracts/UniswapV2.sol#L159
Don't be like Maker.
These functions should be named something that gives the reader a good idea of what their purpose/intent is without having to read and comprehend the entire codebase. Suggestions for better names include:
make => addLiquidity
made => removeLiquidity
move => swap (I would suggest convert, but since the product name is uniSWAP, swap feels more appropriate given the context)
skim => withdrawExcess
sync => iLikeGivingAwayFreeMoney
Same issue for the events, they should be named something meaningful/useful, not unfamiliar jargon.
uniswap-v2-core on ๎ master is ๐ฆ v1.0.1 via โฌข v12.16.3
โฏ yarn
yarn install v1.22.4
[1/5] ๐ Validating package.json...
[2/5] ๐ Resolving packages...
[3/5] ๐ Fetching packages...
[4/5] ๐ Linking dependencies...
[5/5] ๐จ Building fresh packages...
[-/14] โก waiting...
[-/14] โก waiting...
[-/14] โก waiting...
[-/14] โก waiting...
warning Error running install script for optional dependency: "/Users/jayson/Code/uniswap-v2-core/node_modules/scrypt: Command failed.
Exit code: 1
Command: node-gyp rebuild
Arguments:
Directory: /Users/jayson/Code/uniswap-v2-core/node_modules/scrypt
Output:
gyp info it worked if it ends with ok
gyp info using [email protected]
gyp info using [email protected] | darwin | x64
gyp info find Python using Python version 2.7.16 found at \"/System/Library/Frameworks/Python.framework/Versions/2.7/Resources/Python.app/Contents/MacOS/Python\"
gyp info spawn /System/Library/Frameworks/Python.framework/Versions/2.7/Resources/Python.app/Contents/MacOS/Python
gyp info spawn args [
gyp info spawn args '/Users/jayson/.nvm/versions/node/v12.16.3/lib/node_modules/npm/node_modules/node-gyp/gyp/gyp_main.py',
gyp info spawn args 'binding.gyp',
gyp info spawn args '-f',
gyp info spawn args 'make',
gyp info spawn args '-I',
gyp info spawn args '/Users/jayson/Code/uniswap-v2-core/node_modules/scrypt/build/config.gypi',
gyp info spawn args '-I',
gyp info spawn args '/Users/jayson/.nvm/versions/node/v12.16.3/lib/node_modules/npm/node_modules/node-gyp/addon.gypi',
gyp info spawn args '-I',
gyp info spawn args '/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/common.gypi',
gyp info spawn args '-Dlibrary=shared_library',
gyp info spawn args '-Dvisibility=default',
gyp info spawn args '-Dnode_root_dir=/Users/jayson/Library/Caches/node-gyp/12.16.3',
gyp info spawn args '-Dnode_gyp_dir=/Users/jayson/.nvm/versions/node/v12.16.3/lib/node_modules/npm/node_modules/node-gyp',
gyp info spawn args '-Dnode_lib_file=/Users/jayson/Library/Caches/node-gyp/12.16.3/<(target_arch)/node.lib',
gyp info spawn args '-Dmodule_root_dir=/Users/jayson/Code/uniswap-v2-core/node_modules/scrypt',
gyp info spawn args '-Dnode_engine=v8',
gyp info spawn args '--depth=.',
gyp info spawn args '--no-parallel',
gyp info spawn args '--generator-output',
gyp info spawn args 'build',
gyp info spawn args '-Goutput_dir=.'
gyp info spawn args ]
gyp info spawn make
gyp info spawn args [ 'BUILDTYPE=Release', '-C', 'build' ]
SOLINK_MODULE(target) Release/copied_files.node
CC(target) Release/obj.target/scrypt_wrapper/src/util/memlimit.o
CC(target) Release/obj.target/scrypt_wrapper/src/scryptwrapper/keyderivation.o
CC(target) Release/obj.target/scrypt_wrapper/src/scryptwrapper/pickparams.o
CC(target) Release/obj.target/scrypt_wrapper/src/scryptwrapper/hash.o
LIBTOOL-STATIC Release/scrypt_wrapper.a
CC(target) Release/obj.target/scrypt_lib/scrypt/scrypt-1.2.0/lib/crypto/crypto_scrypt.o
CC(target) Release/obj.target/scrypt_lib/scrypt/scrypt-1.2.0/lib/crypto/crypto_scrypt_smix.o
CC(target) Release/obj.target/scrypt_lib/scrypt/scrypt-1.2.0/libcperciva/util/warnp.o
CC(target) Release/obj.target/scrypt_lib/scrypt/scrypt-1.2.0/libcperciva/alg/sha256.o
CC(target) Release/obj.target/scrypt_lib/scrypt/scrypt-1.2.0/libcperciva/util/insecure_memzero.o
CC(target) Release/obj.target/scrypt_lib/scrypt/scrypt-1.2.0/lib/scryptenc/scryptenc_cpuperf.o
LIBTOOL-STATIC Release/scrypt_lib.a
CXX(target) Release/obj.target/scrypt/src/node-boilerplate/scrypt_common.o
CXX(target) Release/obj.target/scrypt/src/node-boilerplate/scrypt_params_async.o
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
In file included from ../src/node-boilerplate/inc/scrypt_params_async.h:28:
In file included from ../src/node-boilerplate/inc/scrypt_async.h:28:
../src/node-boilerplate/inc/scrypt_common.h:39:14: warning: 'Get' is deprecated: Use maybe version [-Wdeprecated-declarations]
N(obj->Get(Nan::New(\"N\").ToLocalChecked())->Uint32Value()),
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8.h:3553:3: note: 'Get' has been explicitly marked deprecated here
V8_DEPRECATED(\"Use maybe version\", Local<Value> Get(Local<Value> key));
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8config.h:328:29: note: expanded from macro 'V8_DEPRECATED'
declarator __attribute__((deprecated(message)))
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
In file included from ../src/node-boilerplate/inc/scrypt_params_async.h:28:
In file included from ../src/node-boilerplate/inc/scrypt_async.h:28:
../src/node-boilerplate/inc/scrypt_common.h:39:63: error: too few arguments to function call, single argument 'context' was not specified
N(obj->Get(Nan::New(\"N\").ToLocalChecked())->Uint32Value()),
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8.h:2707:3: note: 'Uint32Value' declared here
V8_WARN_UNUSED_RESULT Maybe<uint32_t> Uint32Value(
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8config.h:368:31: note: expanded from macro 'V8_WARN_UNUSED_RESULT'
#define V8_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
In file included from ../src/node-boilerplate/inc/scrypt_params_async.h:28:
In file included from ../src/node-boilerplate/inc/scrypt_async.h:28:
../src/node-boilerplate/inc/scrypt_common.h:40:14: warning: 'Get' is deprecated: Use maybe version [-Wdeprecated-declarations]
r(obj->Get(Nan::New(\"r\").ToLocalChecked())->Uint32Value()),
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8.h:3553:3: note: 'Get' has been explicitly marked deprecated here
V8_DEPRECATED(\"Use maybe version\", Local<Value> Get(Local<Value> key));
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8config.h:328:29: note: expanded from macro 'V8_DEPRECATED'
declarator __attribute__((deprecated(message)))
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
In file included from ../src/node-boilerplate/inc/scrypt_params_async.h:28:
In file included from ../src/node-boilerplate/inc/scrypt_async.h:28:
../src/node-boilerplate/inc/scrypt_common.h:40:63: error: too few arguments to function call, single argument 'context' was not specified
r(obj->Get(Nan::New(\"r\").ToLocalChecked())->Uint32Value()),
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8.h:2707:3: note: 'Uint32Value' declared here
V8_WARN_UNUSED_RESULT Maybe<uint32_t> Uint32Value(
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8config.h:368:31: note: expanded from macro 'V8_WARN_UNUSED_RESULT'
#define V8_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
In file included from ../src/node-boilerplate/inc/scrypt_params_async.h:28:
In file included from ../src/node-boilerplate/inc/scrypt_async.h:28:
../src/node-boilerplate/inc/scrypt_common.h:41:14: warning: 'Get' is deprecated: Use maybe version [-Wdeprecated-declarations]
p(obj->Get(Nan::New(\"p\").ToLocalChecked())->Uint32Value()) {}
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8.h:3553:3: note: 'Get' has been explicitly marked deprecated here
V8_DEPRECATED(\"Use maybe version\", Local<Value> Get(Local<Value> key));
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8config.h:328:29: note: expanded from macro 'V8_DEPRECATED'
declarator __attribute__((deprecated(message)))
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
In file included from ../src/node-boilerplate/inc/scrypt_params_async.h:28:
In file included from ../src/node-boilerplate/inc/scrypt_async.h:28:
../src/node-boilerplate/inc/scrypt_common.h:41:63: error: too few arguments to function call, single argument 'context' was not specified
p(obj->Get(Nan::New(\"p\").ToLocalChecked())->Uint32Value()) {}
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8.h:2707:3: note: 'Uint32Value' declared here
V8_WARN_UNUSED_RESULT Maybe<uint32_t> Uint32Value(
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8config.h:368:31: note: expanded from macro 'V8_WARN_UNUSED_RESULT'
#define V8_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
In file included from ../src/node-boilerplate/inc/scrypt_params_async.h:28:
../src/node-boilerplate/inc/scrypt_async.h:53:17: warning: 'Call' is deprecated [-Wdeprecated-declarations]
callback->Call(1, argv);
^
../../nan/nan.h:1739:3: note: 'Call' has been explicitly marked deprecated here
NAN_DEPRECATED inline v8::Local<v8::Value>
^
../../nan/nan.h:104:40: note: expanded from macro 'NAN_DEPRECATED'
# define NAN_DEPRECATED __attribute__((deprecated))
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
../src/node-boilerplate/inc/scrypt_params_async.h:35:36: error: too few arguments to function call, single argument 'context' was not specified
maxtime(info[0]->NumberValue()),
~~~~~~~~~~~~~~~~~~~~ ^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8.h:2704:3: note: 'NumberValue' declared here
V8_WARN_UNUSED_RESULT Maybe<double> NumberValue(Local<Context> context) const;
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8config.h:368:31: note: expanded from macro 'V8_WARN_UNUSED_RESULT'
#define V8_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
../src/node-boilerplate/inc/scrypt_params_async.h:36:39: error: too few arguments to function call, single argument 'context' was not specified
maxmemfrac(info[1]->NumberValue()),
~~~~~~~~~~~~~~~~~~~~ ^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8.h:2704:3: note: 'NumberValue' declared here
V8_WARN_UNUSED_RESULT Maybe<double> NumberValue(Local<Context> context) const;
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8config.h:368:31: note: expanded from macro 'V8_WARN_UNUSED_RESULT'
#define V8_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
../src/node-boilerplate/inc/scrypt_params_async.h:37:36: error: too few arguments to function call, single argument 'context' was not specified
maxmem(info[2]->IntegerValue()),
~~~~~~~~~~~~~~~~~~~~~ ^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8.h:2705:3: note: 'IntegerValue' declared here
V8_WARN_UNUSED_RESULT Maybe<int64_t> IntegerValue(
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8config.h:368:31: note: expanded from macro 'V8_WARN_UNUSED_RESULT'
#define V8_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
^
In file included from ../src/node-boilerplate/scrypt_params_async.cc:4:
../src/node-boilerplate/inc/scrypt_params_async.h:38:39: error: too few arguments to function call, single argument 'context' was not specified
osfreemem(info[3]->IntegerValue())
~~~~~~~~~~~~~~~~~~~~~ ^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8.h:2705:3: note: 'IntegerValue' declared here
V8_WARN_UNUSED_RESULT Maybe<int64_t> IntegerValue(
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8config.h:368:31: note: expanded from macro 'V8_WARN_UNUSED_RESULT'
#define V8_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
^
../src/node-boilerplate/scrypt_params_async.cc:23:8: warning: 'Set' is deprecated: Use maybe version [-Wdeprecated-declarations]
obj->Set(Nan::New(\"N\").ToLocalChecked(), Nan::New<Integer>(logN));
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8.h:3498:3: note: 'Set' has been explicitly marked deprecated here
V8_DEPRECATED(\"Use maybe version\",
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8config.h:328:29: note: expanded from macro 'V8_DEPRECATED'
declarator __attribute__((deprecated(message)))
^
../src/node-boilerplate/scrypt_params_async.cc:24:8: warning: 'Set' is deprecated: Use maybe version [-Wdeprecated-declarations]
obj->Set(Nan::New(\"r\").ToLocalChecked(), Nan::New<Integer>(r));
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8.h:3498:3: note: 'Set' has been explicitly marked deprecated here
V8_DEPRECATED(\"Use maybe version\",
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8config.h:328:29: note: expanded from macro 'V8_DEPRECATED'
declarator __attribute__((deprecated(message)))
^
../src/node-boilerplate/scrypt_params_async.cc:25:8: warning: 'Set' is deprecated: Use maybe version [-Wdeprecated-declarations]
obj->Set(Nan::New(\"p\").ToLocalChecked(), Nan::New<Integer>(p));
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8.h:3498:3: note: 'Set' has been explicitly marked deprecated here
V8_DEPRECATED(\"Use maybe version\",
^
/Users/jayson/Library/Caches/node-gyp/12.16.3/include/node/v8config.h:328:29: note: expanded from macro 'V8_DEPRECATED'
declarator __attribute__((deprecated(message)))
^
../src/node-boilerplate/scrypt_params_async.cc:32:13: warning: 'Call' is deprecated [-Wdeprecated-declarations]
callback->Call(2, argv);
^
../../nan/nan.h:1739:3: note: 'Call' has been explicitly marked deprecated here
NAN_DEPRECATED inline v8::Local<v8::Value>
^
../../nan/nan.h:104:40: note: expanded from macro 'NAN_DEPRECATED'
# define NAN_DEPRECATED __attribute__((deprecated))
^
8 warnings and 7 errors generated.
make: *** [Release/obj.target/scrypt/src/node-boilerplate/scrypt_params_async.o] Error 1
gyp ERR! build error
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack at ChildProcess.onExit (/Users/jayson/.nvm/versions/node/v12.16.3/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:194:23)
gyp ERR! stack at ChildProcess.emit (events.js:310:20)
gyp ERR! stack at Process.ChildProcess._handle.onexit (internal/child_process.js:275:12)
gyp ERR! System Darwin 19.4.0
gyp ERR! command \"/Users/jayson/.nvm/versions/node/v12.16.3/bin/node\" \"/Users/jayson/.nvm/versions/node/v12.16.3/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js\" \"rebuild\"
gyp ERR! cwd /Users/jayson/Code/uniswap-v2-core/node_modules/scrypt
โจ Done in 23.84s.
all good on 11.15
uniswap-v2-core on ๎ master is ๐ฆ v1.0.1 via โฌข v11.15.0
โฏ yarn
yarn install v1.22.4
[1/5] ๐ Validating package.json...
[2/5] ๐ Resolving packages...
[3/5] ๐ Fetching packages...
[4/5] ๐ Linking dependencies...
[5/5] ๐จ Building fresh packages...
โจ Done in 18.42s.
uniswap-v2-core on ๎ master is ๐ฆ v1.0.1 via โฌข v11.15.0 took 18s
โฏ yarn compile
yarn run v1.22.4
$ yarn clean
$ rimraf ./build/
$ waffle .waffle.json
โจ Done in 5.17s.
Nitpick: Consider using uint256 instead of uint for clarity in the code. While unlike C/C++/C#, these types shouldn't ever represent anything other than uint256, I still think there is value in being clear to the reader about the width.
The proper syntax for this is >=10 not ^10. Currently, this will fail if using NodeJS 12. I would submit a PR, but PRs to private repos are hard... you should make this public.
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!
Submitting as a patch here because I am unable to make pull requests. Adding the following to .waffle.json will teach waffle to output klab compatible json files.
From f0abaa0468bca94a258f3c582cbcd803614dd550 Mon Sep 17 00:00:00 2001
From: David Terry <[email protected]>
Date: Mon, 30 Dec 2019 10:38:37 +0100
Subject: [PATCH] emit klab compatibile solc json
see: https://ethereum-waffle.readthedocs.io/en/latest/configuration.html#klab-compatibility
---
.waffle.json | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/.waffle.json b/.waffle.json
index 648160c..6e4046a 100644
--- a/.waffle.json
+++ b/.waffle.json
@@ -1,6 +1,13 @@
{
"solcVersion": "./node_modules/solc",
+ "outputType": "all",
"compilerOptions": {
+ "outputSelection": {
+ "*": {
+ "*": [ "evm.bytecode.object", "evm.deployedBytecode.object", "abi" , "evm.bytecode.sourceMap", "evm.deployedBytecode.sourceMap" ],
+ "": [ "ast" ]
+ }
+ },
"evmVersion": "istanbul",
"optimizer": {
"enabled": true,
--
2.24.1As an example, the exchanges storage variable should be an IUniswapV2[], not an address.
IUniswapV2[] public exchanges;The code is more readable when you have more specific types. Address is sufficiently vague as to make the code difficult to read. Even if you don't interact with the variable as a specific type, it is still a good idea to have a placeholder interface like interface IToken {} that you label variables as just for readability.
Note: contract/interface types are erased at compile time, so there is no runtime overhead to writing this sort of self documenting code.
bool private locked = false;
modifier lock() {
require(!locked, "UniswapV2: LOCKED");
locked = true;
_;
locked = false;
}Personally, I find locked and !locked to be easier to read, understand, and grok than notLocked as a variable. I think this is largely because ! is converted in my brain to not already, so require(notLocked) and require(!locked) end up translating to equivalent statements. It also means I have to mentally spend less time dealing with inverting the concept since the default state is locked = false rather than trying to mentally process the default state being notLocked = true.
Another option would be to rename the variable to unlocked, which is a common English word and thus requires less mental work to process:
bool private unlocked = true;
modifier lock() {
require(unlocked, "UniswapV2: LOCKED");
unlocked = false;
_;
unlocked = true;
}I find this to not be quite as easy to grok as !locked, but notably easier to grok than notLocked.
P.S.: The effort that went into filing this GitHub issue is likely higher than the effort of 100 developers to deal with this code regardless of how it is written, but maybe worth it after 101 developers. ๐
While ERC-777 tokens are compatible with ERC-20 tokens, it is possible (and reasonable) to create an ERC-777-only token. Such a token would not have a transfer method and instead would only have a send method on it. A Uniswap exchange would largely be able to operate the same, only the name of the function being called and parameters passed would change.
I acknowledge that implementing this is a tad tricky. One option would be to have token-type be an input into the factory for each of the tokens in the pair, and the token type would determine which exchange was created. The disadvantage of this is that the user will need to know whether a token is ERC20 or ERC777 in order to locate the exchange for their token.
Another option would be to do a test transfer during exchange creation to try to identify the token type. You could first try .send and if that fails then try .transfer. This greatly complicates the factory, but it would make it so there was only a single exchange type per token.
A third option would be to allow creation of separate ERC20 and ERC777 exchanges, and when a user does an exchange lookup return an array of exchanges. The UI could then give preference to the one with the most liquidity, or potentially present them both in some way.
While I understand why there may be push-back on this, I suspect this is going to become a continuing problem as more and more standards compete. Along with ERC20 and ERC777 there is also ERC721 (which I recognize doesn't fit well with Uniswap, but it shows yet another token standard) and ERC1155 (which could work with Uniswap). I think at some point Uniswap is going to need to support more than just ERC20, especially if the community can successfully move past that horrific standard. Figuring out how to solve this problem for ERC-777 could pave the way to solving the problem more generally so things like ERC-1155 could also work.
Since the owner can change at any time, every time fees need to be paid the transaction must first lookup the fee recipient address on the factory. This results in both a storage lookup and a remote contract call which is somewhat gas expensive.
Before deploying Uniswap Factory/Exchange, first deploy an owned mailbox. Hardcode the address of that mailbox into Uniswap Factory, which would then embed it into Uniswap Exchanges when they are created. This will remove the need to do the lookup which removes both the remote call and the SLOAD. The mailbox itself can have its owner changed at any time, but fee payers can ignore this.
The mailbox can be a simple owned contract that allows delegated execution. There are a number of such already-audited and widely-used contracts that you could use with the simplest being an Owned contract with an execute function on it (note: the recoverable-wallet code has been audited by OpenZeppelin, but you could crib the same code from a number of different projects like Gnosis Safe or various multisig wallets). One caveat is that for some token types (I think 1155 does this) you may need to expose a particular method on the contract to mark it as being a valid receiver of that type of token. In the case of Uniswap fees, you presumably just need to match any such checks Uniswap has since fees will always be in a Uniswap supported token.
Another option for the mailbox could be one that simply allows ERC20, ERC777, ERC1155, and ETH withdraws by the owner. This is even simpler, though it does mean if you receive some weird token as a fee with bizarre requirements for withdraw they may get stuck in a way that an arbitrary executor could recover from.
should it include chainID
can create2 be used with a constructor?
For pausable / whitelist / blacklist tokens
Following the pattern of "put as little code in as possible in the core contracts", you could remove the transfer calls from move and instead just update the reserves. The user would presumably call skim after calling move to withdraw their desired token.
I think this probably has to do with the mineBlock method, or how the timestamps are moving in the underlying evm.
I think we probably should just have a method on the contract that gets the current timestamp so we can override it for tests, per @MicahZoltu's suggestion. This is how I've always done it as well.
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
Django
The Web framework for perfectionists with deadlines.
Laravel
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
Microsoft
Open source projects and samples from Microsoft.
Google
Google โค๏ธ Open Source for everyone.
Alibaba
Alibaba Open Source for everyone
D3
Data-Driven Documents codes.
Tencent
China tencent open source team.