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.

Sign up for GitHub

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

Jump to bottom

Implement Uint256SquareRoot #134

Merged
merged 33 commits into from
Nov 6, 2023
Merged

Implement Uint256SquareRoot #134

merged 33 commits into from
Nov 6, 2023

Conversation

mmk-1
Copy link
Contributor

@mmk-1 mmk-1 commented Oct 23, 2023

resolves issue #131

mmk-1 and others added 19 commits September 22, 2023 17:28
@mmk-1
added UpdatePc tests and fixed a bug in UpdatePc
213cfe4
@mmk-1
Fixed accessing field value without Read()
1e1cfe7
@mmk-1
Merge branch 'main' into main
39f71f6
@mmk-1
fixed failing tests
60194fb
@mmk-1
small refactor for TestUpdatePcJump
df8cd7f
@mmk-1
Merge branch 'NethermindEth:main' into main
98a3d1c
@mmk-1
Merge branch 'NethermindEth:main' into main
1a2280f
@mmk-1
Merge branch 'NethermindEth:main' into main
8dec869
@mmk-1
added SquareRoot hint + test
0447eff
@mmk-1
removed usage of U256
0900990
@mmk-1
fixed the hint for sqrt
1e0188d
@mmk-1
removed unnecessary byte conversion in squareroot hint
1973382
@mmk-1
u256sqrt Execute() function done. need test
b344646
@mmk-1
remove unnecessary comment
e5e5562
@mmk-1
refactoring u256sqrt method
a0eedf5
@mmk-1
fixed uint256sqrt with added test
b793e2e
@mmk-1
add new test case for high bytes
ae53fc3
@mmk-1
refactoring u256sqrt method
e9be2d0
@mmk-1
more refactoring and removed a .clone()
df57e98
@mmk-1 mmk-1 changed the title U256sqrt Implement Uint256SquareRoot Oct 23, 2023
rodrigo-pino
rodrigo-pino reviewed Oct 25, 2023
View reviewed changes
Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

Initial review. It looks good, just some minor issues

pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
@rodrigo-pino
Copy link
Contributor

rodrigo-pino commented Oct 25, 2023 •
edited
Loading

Please include benchmark tests for them as well. Update from main to get the latest update on the hint pkg.

@mmk-1
Copy link
Contributor Author

mmk-1 commented Nov 1, 2023 •
edited
Loading

Please include benchmark tests for them as well. Update from main to get the latest update on the hint pkg.

I added benchmark test as well. Could you check if I did it correctly please? thanks

rodrigo-pino
rodrigo-pino reviewed Nov 1, 2023
View reviewed changes
pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
@rodrigo-pino
Copy link
Contributor

@mmk-1 please fix conflicts so we can merge

@rodrigo-pino rodrigo-pino merged commit 36396a1 into NethermindEth:main Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmjac jmjac jmjac left review comments

@rodrigo-pino rodrigo-pino rodrigo-pino approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mmk-1 @rodrigo-pino @jmjac