Pair symbols

[moodysalem]

Gives pair tokens a good symbol via a call to setPairSymbol(tokenA, tokenB). The method is separate from createPair to reduce risk of breaking createPair this late in the v2 release. However this allows common Uniswap LP shares to have an improved UX in wallets.

The logic for determining a token symbol is as follows:

• attempts to read bytes32 data
• attempts to read string data
• if neither succeeds, or symbol is not implemented, falls back to abbreviated token address (first 6 bytes upper case) for symbol

The token symbols for a pair are concatenated into the pair symbol.

[moodysalem]

~~750k gas for create pair 💀 However we can move the name/symbol computation to the factory, most of this extra gas is probably storing the library bytecode~~ done

[moodysalem]

TODO: reentrancy protection for createPair

[MicahZoltu]

What is the reentrancy attack vector?

[moodysalem]

createPair after this change makes calls into symbol() and name() of the ERC20 tokens, which can call back into createPair

I'm pretty sure you can't do anything dangerous like that but might be worth reentrancy guarding

[MicahZoltu]

I'm not a huge fan of needlessly adding reentrancy protection. It feels like a very blunt-instrument approach to problem solving. I would rather see someone take the time to validate that bad things won't happen if you reenter here.

[moodysalem]

I agree, but how can I prove that bad things won't happen short of formal verification? AFAICT it's totally safe, since create2 will fail if called with the same arguments and there's nothing to be gained calling it with different arguments.

[MicahZoltu]

I'm personally a fan of smart humans reading code and thinking critically as a solution to the problem, rather than formal verification. Just have one or two experienced developers/auditors spend some time actively looking for a particular class of exploits is usually enough to find them in my experience.

[NoahZinsmeister]

afaik we wouldn't need reentrancy protection, as we've declared symbol() and name() as view functions, which as of solc@>=0.5.0 use STATICCALL.

Pure and view functions are now called using the opcode STATICCALL instead of CALL if the EVM version is Byzantium or later. This disallows state changes on the EVM level.

• https://solidity.readthedocs.io/en/v0.5.15/050-breaking-changes.html#semantic-only-changes

[moodysalem]

afaik we wouldn't need reentrancy protection, as we've declared symbol() and name() as view functions, which as of solc@>=0.5.0 use STATICCALL.

Pure and view functions are now called using the opcode STATICCALL instead of CALL if the EVM version is Byzantium or later. This disallows state changes on the EVM level.

• https://solidity.readthedocs.io/en/v0.5.15/050-breaking-changes.html#semantic-only-changes

Neat! I’ll change it to staticcall

[moodysalem]

https://github.com/Uniswap/uniswap-v2-core/pull/73#issuecomment-617856313