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

Pythonic Alternative to Construct-based Code #56

Closed
wants to merge 6 commits into from
Closed

Pythonic Alternative to Construct-based Code #56

wants to merge 6 commits into from

Conversation

suhasasumukh
Copy link
Contributor

This PR replaces the original construct-based code with a more readable and flexible Pythonic alternative.
It uses data classes, the struct module, and custom pack/unpack functions while preserving the original functionality.

This PR fixes #44

Rewrote the layouts in more Pythonic way
@suhasasumukh suhasasumukh changed the title structures Pythonic Alternative to Construct-based Code Jan 28, 2024
@GitBolt
Copy link
Collaborator

GitBolt commented Jan 28, 2024

Did you test it? The code is not sitting there, it is being used in Instructions.
Right now the PR does not make the layouts compatible with instructions. Can you do that and ensure its working fine?

@suhasasumukh
Copy link
Contributor Author

Did you test it? The code is not sitting there, it is being used in Instructions. Right now the PR does not make the layouts compatible with instructions. Can you do that and ensure its working fine?

I dont see any conflict from my side & I have made it compatible with instructions

@GitBolt
Copy link
Collaborator

GitBolt commented Jan 28, 2024

Did you test it? The code is not sitting there, it is being used in Instructions. Right now the PR does not make the layouts compatible with instructions. Can you do that and ensure its working fine?

I dont see any conflict from my side & I have made it compatible with instructions

Instructions file is not updated tho. How have you made it compatible with it without modifying instructions as per changes made? The functions defined in the layouts file aren't being used to decode instructions in the instructions.py file I mentioned above.

@suhasasumukh
Copy link
Contributor Author

Did you test it? The code is not sitting there, it is being used in Instructions. Right now the PR does not make the layouts compatible with instructions. Can you do that and ensure its working fine?

I dont see any conflict from my side & I have made it compatible with instructions

Instructions file is not updated tho. How have you made it compatible with it without modifying instructions as per changes made? The functions defined in the layouts file aren't being used to decode instructions in the instructions.py file I mentioned above.

ADVANCE_NONCE_ACCOUNT has no arguments, you can keep Pass. If it requires handling in the future, you can replace it

@GitBolt
Copy link
Collaborator

GitBolt commented Jan 29, 2024

Did you test it? The code is not sitting there, it is being used in Instructions. Right now the PR does not make the layouts compatible with instructions. Can you do that and ensure its working fine?

I dont see any conflict from my side & I have made it compatible with instructions

Instructions file is not updated tho. How have you made it compatible with it without modifying instructions as per changes made? The functions defined in the layouts file aren't being used to decode instructions in the instructions.py file I mentioned above.

ADVANCE_NONCE_ACCOUNT has no arguments, you can keep Pass. If it requires handling in the future, you can replace it

I'm not sure what you mean here. I never mentioned about advance nonce account. I said that the changes that you have made look fine on surface, but they aren't tested. Because the layouts are supposed to be used in the instructions.py file to build instructions for all these specific actions. Making it Pythonic does not mean replacing data structures coming from struct lib and replacing them with class or dataclasses. It must compile down to the specific byte code that is accurate representation of the instruction, which struct is doing by default. For example this build method ultimately compiles down our code into byte code.

I really don't understand what you are doing here. The code won't even work. You are using plenty of things without defining them. Please ensure your code is working properly before proposing changes. Closing this PR for now.

@GitBolt GitBolt closed this Jan 29, 2024
@GitBolt GitBolt added the invalid This doesn't seem right label Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite layouts in more Pythonic way
2 participants