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

Fix private code imported from SW lib #291

Open
gaetbout opened this issue Apr 12, 2024 · 11 comments
Open

Fix private code imported from SW lib #291

gaetbout opened this issue Apr 12, 2024 · 11 comments
Labels
enhancement Enhancement of the code, not introducing new features.

Comments

@gaetbout
Copy link
Collaborator

// TODO Remove all done, it was copied from Starkware's library. There must be a better way
Fix this todo.

@gaetbout gaetbout added the enhancement Enhancement of the code, not introducing new features. label Apr 12, 2024
Copy link

There hasn't been any activity on this issue recently, and in order to prioritize active issues, it will be marked as stale.
Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by leaving a 👍
Because this issue is marked as stale, it will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

@github-actions github-actions bot added the stale No activity for quite some time. label May 13, 2024
@gaetbout
Copy link
Collaborator Author

Ping

@github-actions github-actions bot removed the stale No activity for quite some time. label May 14, 2024
Copy link

There hasn't been any activity on this issue recently, and in order to prioritize active issues, it will be marked as stale.
Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by leaving a 👍
Because this issue is marked as stale, it will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

@github-actions github-actions bot added the stale No activity for quite some time. label Jun 14, 2024
@gaetbout
Copy link
Collaborator Author

Pong

@github-actions github-actions bot removed the stale No activity for quite some time. label Jun 15, 2024
Copy link

There hasn't been any activity on this issue recently, and in order to prioritize active issues, it will be marked as stale.
Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by leaving a 👍
Because this issue is marked as stale, it will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

@github-actions github-actions bot added the stale No activity for quite some time. label Jul 15, 2024
@gaetbout
Copy link
Collaborator Author

.

@github-actions github-actions bot removed the stale No activity for quite some time. label Jul 16, 2024
@harsh-ps-2003
Copy link
Contributor

harsh-ps-2003 commented Jul 21, 2024

@gaetbout
I see that because the method is not public in the core library, we have copied the method. I don't get Fix private code imported - its not imported, its defined in the file itself! There must be a better way - so we are trying to reimplement the methods in a better way, so there must be some problem with this copy pasted implementation. Can you elaborate the problem?

A better way to implement this that I can think about is using bitwise left shift directly instead of match statement with hardcoded values for each shift. also directly using the operator will allow us to handle any shift amount (up to max allowed by the type ofc) which would be better than total 31 bytes that we are handling using match. But I think << operator is unavailable! so I might need to have a custom left shift method itself! Wanna see it in a PR? edit: tried it, got a lot of outofgas errors :(

But I don't find anything inherently wrong with this standard implementation itself.

Sorry if the question is dumb, new to Starknet and Cairo in general! Thanks

@gaetbout
Copy link
Collaborator Author

My point is that we shouldn't have function copy pasted from SW repo.
I believe reworking the logic, as you suggested, might be a correct fix.
Or we can ask SW to make that fn public

@harsh-ps-2003
Copy link
Contributor

If SW can make the methods public, that would be sweet!

Copy link

There hasn't been any activity on this issue recently, and in order to prioritize active issues, it will be marked as stale.
Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by leaving a 👍
Because this issue is marked as stale, it will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

@github-actions github-actions bot added the stale No activity for quite some time. label Aug 21, 2024
@gaetbout
Copy link
Collaborator Author

Ping

@github-actions github-actions bot removed the stale No activity for quite some time. label Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of the code, not introducing new features.
Projects
None yet
Development

No branches or pull requests

2 participants