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

More Truffle examples - all now @ solc 0.5.0 #33

Merged
merged 25 commits into from
Dec 3, 2018
Merged

Conversation

gskapka
Copy link
Contributor

@gskapka gskapka commented Nov 10, 2018

What this PR is:

Adding more of the Truffle-versioned examples for folk to download and run, showing them how to use our examples in a full-blown Truffle (5) environment, with testing and everything.


To Do:

@riccardopersiani Please could you pull this then test each of the examples in turn making sure the tests all pass for you too. The jpgs in each repo are from the tests passing on my machine, so ∴ should all pass for you too.

Once you've done this, please report back an issues, suggestions for improvements, notes etc so I can deal with them, and then finally @D-Nice can merge.


Notes:

Included in each of the example folders is a jpg of the passing tests on my machine, from fresh repo-pulls. These jpgs will appear correctly in the README once this PR is merged, but will yet not display properly in this PR due to wrong relative paths.

Each folder is intended to be a stand-alone, download-able working example, hence each of them having their own versions of the oraclizeAPI, archive.zip's and other such files. I've not used symlinks to the actual API for this reason too.

Full disclosure: Also making this PR & updates to test my split-ssh qube set up is working correctly! 🤣

@riccardopersiani
Copy link
Contributor

riccardopersiani commented Nov 12, 2018

Outcome:

All the tests have been passed successfully once.

screenshot 2018-11-12 at 09 50 48


Notes:

MacOS

@D-Nice @gskapka, I experienced some weird behaviours due probably to npm/truffle/computation/engine:

after the first npm install a module was not installed;

image

after approximately 5 mins, looks like the ethereum-bridge is kind of going down or not answering properly;

image

Maybe we should inform users about this potential behaviour...

if a bunch of tests are done consecutively, something fails (probably the computation?).

some images in the folders not showing.

@gskapka
Copy link
Contributor Author

gskapka commented Nov 12, 2018

So the URL-Requests always runs through cleanly the first time, and then one or occasionally two tests fail on a subsequent run (if done immediately after) due to the computation itself not returning the correct thing. If instead you wait a while, the test-suite runs through cleanly again.

I had no idea why but didn't look into debugging it too hard since the tests work on the Truffle/Bridge side of things and pass as they ought.

Re the npm thing, that's a local issue and nothing to do with the repo.


To do @riccardopersiani

Per the original request, could you please run all the truffle examples to make sure they all work as expected on MacOS?

Copy link
Contributor

@riccardopersiani riccardopersiani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests are successfully passed:

  • ✅ bitcoin-balance;
  • ✅ delegated-math;
  • ✅ diesel-price;
  • ✅ kraken-price-ticker;
  • ✅ streamr;
  • ✅ swarm;
  • ✅ url-requests;
  • ✅ wolfram-alpha;
  • ✅ youtube-views.

Copy link
Contributor

@D-Nice D-Nice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any nice way to dedupe the oraclizeAPI file from each of the examples, whether it be via a symbolic link or submodule? It seems a bit inefficient to have it repeated multiple times in the same repo.

@D-Nice
Copy link
Contributor

D-Nice commented Nov 15, 2018

Even the example contract files could be symlinked to the current master ones, helping with maintainability if they need to be updated (albeit maybe not the smartest thing to do now with 0.5 out, since we'll probably want to make thes ones in master compatible with 0.5, not sure how compatible your truffle integrations are going to be with those upon update?).

@gskapka
Copy link
Contributor Author

gskapka commented Nov 15, 2018

Will work on the API update to 0.5 first then rebase this PR on it to make it work with the latest versions.

Re symlinking the API - should be no issue, however I didn't do so since I wanted the truffle examples here to stand alone/be truffle-boxable in future. Because these Oraclize examples are an entry-point for many people new to smart-contracts I wanted the truffle versions to be as self-contained as possible, and as simple as possible, which using sub-modules would kind of go against.

There's also the issue of the symlinks not working on windows machines...? Note also that the example contracts inside these new truffle ones are also updated to current best practices (emit keyword etc) and are not the same as the contracts in the base repo for that reason.


So thoughts going forward @D-Nice & @riccardopersiani?

Option 1) Do we keep these as stand-alone? It's more work to maintain (though hardly since these will be semver locked to working versions of everything)...

Option 2) Or do we update the contracts in the main repo to be as current as possible (once the API is current) and then symlink everything in the truffle versions?

@D-Nice
Copy link
Contributor

D-Nice commented Nov 15, 2018

Yes, you're correct about the Windows incompatibility, which slipped my mind yesterday, and I was going to mention today. I think that may make it a deal-breaker, so let's stick with the stand-alone philosophy. But a script to replace duplicate files from root would help with maintanability.

@gskapka
Copy link
Contributor Author

gskapka commented Nov 15, 2018

Stand-alone with some automation it is then!

@gskapka
Copy link
Contributor Author

gskapka commented Nov 26, 2018

Achtung!

All the truffle examples herein are now upgraded to work with the 0.5 version of our API. All tests are still passing. However, could @riccardopersiani please run the new versions through tests per their README.mds to confirm the prior assertion.

@riccardopersiani
Copy link
Contributor

riccardopersiani commented Nov 26, 2018

@gskapka I tested every example successfully and checked every image in the corresponding README.md.

@gskapka
Copy link
Contributor Author

gskapka commented Nov 27, 2018

Update:

✔️ Script added to copy examples from the root of the repo to the correct places in the truffle examples dir.

✒️ NOTE: Of course if you run this script once this branch has been merged the truffle tests won't pass anymore due to the examples currently being sat at solc <= 0.5.0. However, I've made a branch to PR that updates the remainder of the repo to the new compiler version once this PR is merged.


@D-Nice Can you give this a quick glance as I think it's ready to merge.

@D-Nice
Copy link
Contributor

D-Nice commented Nov 27, 2018

LGTM

All the new truffle examples will target 0.5 then. Is it more appropriate to wait for this PR provable-things/ethereum-api#67 to be resolved and merged, so that any changes to it can be reflected for the examples before we merge? Other than that point, it appears good and ready to merge.

@gskapka
Copy link
Contributor Author

gskapka commented Nov 28, 2018

✔️ Agreed re waiting until the main API has been finished. So ∴ will await that being settled and then merge this.


✒️ Note:

I figure that we should target everything to the new 0.5 compiler since it's a breaking one. I don't see any reason why we shouldn't update everything (I've already made a branch updating the main examples in the repo to 0.5) and simply tag it, so if people want earlier versions then can just go back in the repo's history. Thoughts @D-Nice?

@gskapka gskapka changed the title More Truffle examples More Truffle examples - all now @ solc 0.5.0 Nov 28, 2018
@D-Nice
Copy link
Contributor

D-Nice commented Nov 30, 2018

Yes, tagging it is a good call, allowing for those needing to refer to older versions.

@D-Nice
Copy link
Contributor

D-Nice commented Dec 3, 2018

@D-Nice
Copy link
Contributor

D-Nice commented Dec 3, 2018

Merging

@D-Nice D-Nice merged commit e983db7 into master Dec 3, 2018
@D-Nice
Copy link
Contributor

D-Nice commented Dec 3, 2018

@gskapka delete the branch once able/deemed unnecessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants