Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: connector v1.3 review #88

Open
wants to merge 64 commits into
base: master
from

Conversation

@D-Nice
Copy link
Contributor

commented May 23, 2019

No description provided.

@D-Nice D-Nice requested a review from riccardopersiani May 23, 2019
@D-Nice D-Nice removed the request for review from riccardopersiani May 23, 2019
@D-Nice

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

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.

oraclizeAPI_0.4.25.sol Show resolved Hide resolved
modifier oraclizeAPI {
if((address(OAR)==0)||(getCodeSize(address(OAR))==0))
oraclize_setNetwork(networkID_auto);

if(address(oraclize) != OAR.getAddress())
oraclize = OraclizeI(OAR.getAddress());

oraclizeBytes = OraclizeIBytes(OAR.getAddress());

This comment has been minimized.

Copy link
@D-Nice

D-Nice May 23, 2019

Author Contributor

can save some ops by just doing = address(oraclize) here, no?

This comment has been minimized.

Copy link
@gskapka

gskapka May 28, 2019

Contributor

Ahh wait yes, because the line before ensures oraclize is set. Good spot!

oraclizeAPI_0.4.25.sol Show resolved Hide resolved
oraclizeAPI_0.4.25.sol Show resolved Hide resolved
oraclizeAPI_0.4.25.sol Show resolved Hide resolved
@D-Nice

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

ping @gskapka

@gskapka

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

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.

Copy link
Contributor Author

left a comment

Additional points needing addressing

oraclizeAPI_0.4.25.sol Outdated Show resolved Hide resolved
oraclizeAPI_0.5.sol Outdated Show resolved Hide resolved
oraclizeAPI_0.4.25.sol Outdated Show resolved Hide resolved
@gskapka gskapka force-pushed the feat/connector-v1.3 branch 2 times, most recently from 59ee890 to 0082e85 Jun 10, 2019
@D-Nice D-Nice marked this pull request as ready for review Jun 10, 2019
@D-Nice D-Nice requested a review from riccardopersiani Jun 10, 2019
@D-Nice

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

LGTM as of 6a0b495

rest please review as well

@D-Nice D-Nice requested a review from gskapka Jun 10, 2019
Copy link
Contributor

left a comment

Of your commits, I couldn't approve of the specifier order consistency more! Wohoo!

So yeah, LGTM too.

Have given @riccardopersiani an in-person overview of the changes because if you look at the diffs for the whole branch, they don't even load due to their size! It's given him a fighting chance at a review though & he's on it.

Copy link

left a comment

Review still need to be finished

oraclizeAPI_0.4.25.sol Show resolved Hide resolved
oraclizeAPI_0.4.25.sol Show resolved Hide resolved
oraclizeAPI_0.4.25.sol Show resolved Hide resolved
oraclizeAPI_0.4.25.sol Show resolved Hide resolved
oraclizeAPI_0.4.25.sol Outdated Show resolved Hide resolved
oraclizeAPI_0.4.25.sol Show resolved Hide resolved
oraclizeAPI_0.5.sol Outdated Show resolved Hide resolved
oraclizeAPI_0.5.sol Outdated Show resolved Hide resolved
oraclizeAPI_0.5.sol Outdated Show resolved Hide resolved
oraclizeAPI_0.4.25.sol Outdated Show resolved Hide resolved
oraclizeAPI_0.5.sol Outdated Show resolved Hide resolved
oraclizeAPI_0.5.sol Outdated Show resolved Hide resolved
@gskapka

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Changes per @riccardopersiani review pushed & include:

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

✔️ 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.

oraclizeAPI_0.5.sol Outdated Show resolved Hide resolved
@gskapka gskapka requested a review from riccardopersiani Jun 11, 2019
Copy link

left a comment

LGTM

@D-Nice

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

In need of rebase (draft #94)

TODOs post-rebase

  • update copyright in rebased oraclize api
  • update provableAPI with changes of this PR in single commit
  • ensure natspec fix ends up included post-merge
gskapka added 8 commits Mar 6, 2019
...which also makes the visibility specifiers & fxn modifiers easier to
see.
Now users can query prices for their (or others') contracts!
gskapka and others added 18 commits Apr 29, 2019
...since it's free w/r/t compile size if functions go unused.
... & also cleans up the natspec comments surrounding the various
overload types.
... & also update the natspec comments to make the API easier to
navigate.
reverting changes I requested for consistency with 0.5 and previous iterations
@D-Nice D-Nice force-pushed the feat/connector-v1.3 branch from a8371e4 to 3d358e4 Jul 3, 2019
@D-Nice

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

rebased and matches #94

D-Nice added 2 commits Jul 3, 2019
also includes various fixes/consistency unto oraclize apis
@D-Nice D-Nice self-assigned this Jul 3, 2019
@D-Nice D-Nice requested a review from riccardopersiani Jul 3, 2019
@D-Nice

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

Only commit 35d8933

should need review @riccardopersiani

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

Copy link

left a comment

Added just very small notes.

For the rest LGTM. Good job!

oraclizeAPI_0.4.25.sol Outdated Show resolved Hide resolved
oraclizeAPI_0.4.25.sol Outdated Show resolved Hide resolved
oraclizeAPI_0.4.25.sol Outdated Show resolved Hide resolved
oraclizeAPI_0.4.25.sol Outdated Show resolved Hide resolved
courtesy @riccardopersiani

Co-Authored-By: Riccardo Persiani <r.persiani92@gmail.com>
@D-Nice

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.