search

makerdao / univ2-lp-oracle

GNU Affero General Public License v3.0
23 stars 13 forks source link

kmbarry1 review #7

Closed kmbarry1 closed 3 years ago

kmbarry1 commented 3 years ago

Using this PR as a place to discuss my review comments (and ultimately submit fixes).

Q's to start us off, beyond the code changes:

1) Any particular reason hop is a uint16 instead of uint32 to match zzz? I guessing maybe a storage alignment optimization? Otherwise it's a bit weird to use different units.

2) How finely have we scrutinized for rounding errors and stuff? E.g. the sqrt algorithm looks fine from a pure math perspective, but I'd need to spend more time to convince myself that the rounding errors inherent in finite precision don't present any challenges.

NiklasKunkel commented 3 years ago

(1) This is just legacy from the OSM where hop was defined as a uint16 (indeed for storage alignment reasons).

(2) I've done very little scrutinizing around rounding errors. The sqrt algo was lifted from the Uniswap contracts.

kmbarry1 commented 3 years ago

Okay for (1) my personal recommendation would be to make hop a uint32, which matches zzz and does not cause spillover into another storage slot, as I think it's a little more logical for a reader. But not a big deal either way.

If the math functions have been heavily scrutinized elsewhere, they're probably fine.