provable-things / ethereum-api

Provable API for Ethereum smart contracts
https://docs.provable.xyz/#ethereum
MIT License
800 stars 428 forks source link

feat: connector v1.3 review #88

Open D-Nice opened 5 years ago

D-Nice commented 5 years ago

Regarding this, in the future, it may make more sense to do any stylistic refactors first, and merge them in, as it pollutes the code review, and can enter a review hypnosis where what I'm thinking are stylistic changes may be real code changes.

D-Nice commented 5 years ago

ping @gskapka

gskapka commented 5 years ago

Agreed re the stylistic changes btw. Didn't mean to accidentally do both in tandem, but I did :/ Will ensure to separate the two concerns in future if both sorts of changes are needed. Have pushed all the fixes to the above comments.

D-Nice commented 5 years ago

LGTM as of https://github.com/oraclize/ethereum-api/pull/88/commits/6a0b495dca8c91e69e098f719bccb22c98b11e3b

rest please review as well

gskapka commented 5 years ago

Changes per @riccardopersiani review pushed & include:

:heavy_check_mark: All 24 uint related things in 0.5API now solved with a better regex of uint\(256\)\@!\(8\)\@!\(160\)\@! in the find & replace.

:heavy_check_mark: address _address param in getPrice overloads now elucidated via renaming to address _contractToQuery.

All comments made re stylistic changes pertaining to 0.4.25 were ignored since the file itself should have been ignored in that regard during review.

D-Nice commented 5 years ago

In need of rebase (draft #94)

TODOs post-rebase

D-Nice commented 5 years ago

rebased and matches https://github.com/provable-things/ethereum-api/pull/94

D-Nice commented 5 years ago

Only commit https://github.com/provable-things/ethereum-api/pull/88/commits/35d89331bc710ccacd4f733e1d348babdc39f758

should need review @riccardopersiani

If there is anything questionable, please mention and request review from @gskapka in addition

D-Nice commented 5 years ago

resolved