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

Using SafeCast for all downcasting #267

Merged
merged 7 commits into from
Nov 4, 2019
Merged

Using SafeCast for all downcasting #267

merged 7 commits into from
Nov 4, 2019

Conversation

bh2smith
Copy link
Contributor

@bh2smith bh2smith commented Nov 2, 2019

Closes #212 - by using specific commit hash of OpenZeppelin dependency and using its new SafeCast security tool!

TestPlan: checkout this branch, run npm install followed by truffle test (with an instance of ganache–cli running).

package.json Outdated Show resolved Hide resolved
package.json Outdated
@@ -46,7 +46,7 @@
"ethereumjs-util": "^6.0.0",
"fast-memoize": "^2.5.1",
"merkletreejs": "0.0.22",
"openzeppelin-solidity": "2.1.1",
"openzeppelin-solidity": "OpenZeppelin/openzeppelin-contracts#master",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer a concrete commit, instead of a changing reference such as master

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you check one commit behind, you'll see that neither of these options are working.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't seem to be running npm install on travis for some weird reason. I think that's why it is failing.

Also, is SafeCast included in a branch or tagged version (e.g. v2.5rc or such). Then we could make it explicit that we are waiting on a specific version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't seem to be running npm install on travis for some weird reason.

I thought this might be the case, but second guessed myself, cuz I thought "How could anything be working if we don't run npm install?"

Is SafeCast included in a branch or tagged version

I'll check it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, at this time, there are no branch tags on OZ since their most recent release (which did not yet include these changes)

https://github.com/OpenZeppelin/openzeppelin-contracts/tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nlordell - I like the idea of using the shorter hash. Do we know if this works?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bh2smith absolutely none, I just imagine it would as it probably uses git under the hood to resolve refs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does one know the exact number of characters that constitute a legit "short-hash"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works, thanks @nlordell -

"OpenZeppelin/openzeppelin-contracts#2c11ed59"

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, there is no need for npm install in the travis file as the install script section was not overloaded. Note that travis calls npm ci instead of npm install which is usually better but uses your project-lock.json file - which explains why SafeCast.sol was missing. What surprises me is that npm ci should fail if package-lock.json and package.json don't match but didn't in this case.

@bh2smith bh2smith requested a review from josojo November 4, 2019 11:33
Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@josojo josojo left a comment

Choose a reason for hiding this comment

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

A Beauty!

@bh2smith bh2smith merged commit d388f8e into master Nov 4, 2019
@bh2smith bh2smith deleted the 212/use_safecast branch November 4, 2019 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PoC] Downcasting from u256 to u128
4 participants