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

Add bounds check on UnsignedLongs #312

Closed
jrhea opened this issue Feb 14, 2019 · 6 comments · Fixed by #550
Closed

Add bounds check on UnsignedLongs #312

jrhea opened this issue Feb 14, 2019 · 6 comments · Fixed by #550
Assignees
Labels
blocked by research 👩‍🏫 This issue or pull request is blocked by incomplete spec enhancement 🕵️‍♀️ New feature or request

Comments

@jrhea
Copy link
Contributor

jrhea commented Feb 14, 2019

Description

I want to augment UnsignedLong's capabilities with extension methods, but Java doesn't support these natively. Investigate the use of Manifold (or some other method) to give us the desired functionality.

http://manifold.systems/docs.html#extension-method-basics

I would like to have an easy way to do a bounds check on UnsignedLongs automatically when we do an UnsignedLong.plus() or UnsignedLong.minus().

Acceptance Criteria

Add the following extension methods to UnsignedLong:

  • tryMinus()
  • tryPlus()
  • tryLongValue()
  • tryIntValue()
  • integrate into the rest of Artemis

Note: tryMinus() and tryPlus() methods should throw an error if an underflow or overflow is detected

@jrhea jrhea added enhancement 🕵️‍♀️ New feature or request help wanted 🆘 Extra attention is needed labels Feb 14, 2019
@atoulme
Copy link
Contributor

atoulme commented Feb 14, 2019

Here is a proposal via Cava: ConsenSysMesh/cava#173

@jrhea jrhea added blocked by research 👩‍🏫 This issue or pull request is blocked by incomplete spec and removed help wanted 🆘 Extra attention is needed labels Feb 14, 2019
@jrhea
Copy link
Contributor Author

jrhea commented Feb 14, 2019

Here is the issue where this will be debated:

ethereum/consensus-specs#626

@jrhea
Copy link
Contributor Author

jrhea commented Mar 7, 2019

I'm thinking that we might want to switch to Cava's uint64 type now that it is backed by long and has implemented some convenience methods for overflow detection

@akhila-raju
Copy link
Contributor

@jrhea, has this been completed or is it still an open issue?

@akhila-raju akhila-raju changed the title UnsignedLong needs help! Add bounds check on UnsignedLongs Mar 28, 2019
@jrhea
Copy link
Contributor Author

jrhea commented Mar 29, 2019

@akhila-raju it is still an open issue. I think we want to replace unsigned long with uint64 from cava. Antoine created an implementation that didn't rely on big integer

@akhila-raju
Copy link
Contributor

For the sake of easier code readability and maintainability, we have decided to switch to long, since mathematical functions (addition, subtraction, multiplication, division, modulo, amongst others) are already supported by Java for longs. Every time we use an UnsignedLong to index an array, it has to be cast to an integer. Java already has long to int cast support.

If we were to keep UnsignedLong, there would be many files we would have to maintain, including SSZ implementation for UnsignedLong, all supporting mathematical functions implemented for UnsignedLong, etc. It's unnecessary overhead since integer indexes are the major limiting factor of using UnsignedLong.

Thus, to keep it simpler, we have decided, going forward, to use Java's native long rather than UnsignedLong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked by research 👩‍🏫 This issue or pull request is blocked by incomplete spec enhancement 🕵️‍♀️ New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants