Position descriptor #353
Position descriptor #353
Conversation
|
|
||
| int256 constant DENOMINATOR_MOST = -300; | ||
| int256 constant DENOMINATOR_MORE = -200; | ||
| int256 constant DENOMINATOR_2 = -150; |
There was a problem hiding this comment.
why cant WETH just use DENOMINATOR as well? I feel like ETH/WETH pools are going to be very unusual, and the pricing of them probably shouldnt matter?
There was a problem hiding this comment.
i made them different because the native currency might not always be ETH
There was a problem hiding this comment.
would it make sense to pass in something like wrappedNative instead of WETH? that way for CELO, Polygon, and zksync (if we ever deploy to them) we would be able to cover both addresses?
There was a problem hiding this comment.
and then that way wrappedNative and native have the same priority.
we would then not care about WETH priority on those chains.
so on those chains it could be possible to display price like WETH/CELO
There was a problem hiding this comment.
mm thats true ok. I just hate the name DENOMINATOR_2 😂 please can you rename it? then we can keep it hahah
There was a problem hiding this comment.
oh i was thinking we could just remove it altogether
| buffer[params.sigfigIndex--] = "."; | ||
| } | ||
| buffer[params.sigfigIndex] = bytes1(uint8(48 + (params.sigfigs % 10))); | ||
| // can overflow when sigfigIndex = 0 |
There was a problem hiding this comment.
why is overflow desirable?
src/PositionDescriptor.sol
Outdated
| tokenId: tokenId, | ||
| quoteCurrency: quoteCurrency, | ||
| baseCurrency: baseCurrency, | ||
| quoteCurrencySymbol: quoteCurrency.isAddressZero() |
There was a problem hiding this comment.
i think id rather this logic was inside the namer. Like SafeCurrencyNamer and then in there check .isAddressZero() to abstract away from this contract
There was a problem hiding this comment.
so have the isAddressZero check inside of the namer? that's not directly related to name though. Plus, the isAddressZero check is also used for decimals
There was a problem hiding this comment.
I guess I'm wondering if SafeERC20Namer should be called like SafeCurrencyMetadata or something?
and then it can include
- logic for checking for isAddressZero and returning information for native
- try-catching decimals (from other comment) and returning 0 if not
src/PositionDescriptor.sol
Outdated
| : SafeERC20Namer.tokenSymbol(Currency.unwrap(baseCurrency)), | ||
| quoteCurrencyDecimals: quoteCurrency.isAddressZero() | ||
| ? 18 | ||
| : IERC20Metadata(Currency.unwrap(quoteCurrency)).decimals(), |
There was a problem hiding this comment.
why is safe decimal fetching not needed?
There was a problem hiding this comment.
not sure because for name we check to see if its returned in bytes and then convert it to string.
i guess we could do something similar for uint8?
There was a problem hiding this comment.
not sure how likely that decimals will be in bytes tho
There was a problem hiding this comment.
but what if the token doesnt have a decimals function? they dont have to. I think you have to default it to 0
There was a problem hiding this comment.
if you look at the spec the function is optional. So you can do a try-catch and default to 0 or something
src/libraries/SafeERC20Metadata.sol
Outdated
| /// @title SafeERC20Metadata | ||
| /// @notice can produce symbols and decimals from inconsistent or absent ERC20 implementations | ||
| /// @dev Reference: https://github.com/Uniswap/solidity-lib/blob/master/contracts/libraries/SafeERC20Namer.sol | ||
| library SafeERC20Metadata { |
There was a problem hiding this comment.
Just a tiny nit to rename it SafeCurrencyMetadata now that it includes native?
WIP