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

Refactor types and utils #42

Open
shrimalmadhur opened this issue Oct 16, 2023 · 9 comments
Open

Refactor types and utils #42

shrimalmadhur opened this issue Oct 16, 2023 · 9 comments
Labels
good first issue Good for newcomers p2 mid priority
Milestone

Comments

@shrimalmadhur
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
We need to refactor types and utils so that it's in some standard way and not create circular dependencies.

Describe the solution you'd like

  • Yet to define

Describe alternatives you've considered

Additional context

@shrimalmadhur shrimalmadhur added the p2 mid priority label Oct 16, 2023
@shrimalmadhur shrimalmadhur added this to the v0.1.0 milestone Oct 16, 2023
@samlaf samlaf added the good first issue Good for newcomers label Oct 27, 2023
@RohanShrothrium
Copy link

Can I pick this up?

@samlaf
Copy link
Collaborator

samlaf commented Oct 31, 2023

Can I pick this up?

By all means! Do you want to first describe the solution you're thinking of? Like break it down into how you would refactor the files etc? Otherwise, feel free to create a PR, but we might ask you to move things around.

@RohanShrothrium
Copy link

Can I pick this up?

By all means! Do you want to first describe the solution you're thinking of? Like break it down into how you would refactor the files etc? Otherwise, feel free to create a PR, but we might ask you to move things around.

@samlaf I haven't really given it a thought, but will update the issue with what I think can be done!

@RohanShrothrium
Copy link

@samlaf I went through the code.
So here are a bunch of things I think are important to refactor:

  1. The util functions used for validating types should be a common file for the types module as I believe this is not going to be used anywhere apart from validating a "type".
  2. There are a few unused functions in the utils itself, not sure if this has been written for future scope. Do we want to remove this? Internal functions used for utils should not start with caps and should remain internal to the module. Eg ReadFile function. But this again calls for answering the previous question, do we want to have unused functions for future scope? I believe these should be implemented when required.
  3. Certain internal functions used in validation by different types should be moved to a common.go under the types module itself.

Let me know what you think about this!

@samlaf
Copy link
Collaborator

samlaf commented Nov 2, 2023

@RohanShrothrium

  1. agree
  2. agree. ReadFile is completely useless... it's literally wrapping over os.ReadFile with the same interface. Please remove it as well as any you see fit. Only one thing to be careful, it's possible some functions are already used in https://github.com/Layr-Labs/incredible-squaring-avs. I would suggest creating a vscode workspace with both repos and making sure the functions you remove/make private are not being used there.
  3. sgtm! We aren't following any type of pkg directory structure like https://github.com/golang-standards/project-layout so feel free to recommend anything you think would be useful!

@RohanShrothrium
Copy link

Hi, @samlaf!
I'm just a bit preoccupied with some work and Devconnect will take this up once I am back!

@0xpanicError
Copy link

Hey @RohanShrothrium
Are you still working on this? If not then I'd like to pick it up.

@RohanShrothrium
Copy link

@0xpanicError I totally forgot about this. Will raise a PR today ser!

@RohanShrothrium
Copy link

@samlaf I refactored the code with respect to what I mentioned. I will commit this but want to add the following additionally.
There are types defined in the crypto module. I don't see any problem here per se, but there are a few types that are imported again in the types module or in other modules. I will add this to the types module. Let me know if this sounds good.

RohanShrothrium added a commit to RohanShrothrium/eigensdk-go that referenced this issue Dec 6, 2023
Signed-off-by: Rohan Shrothrium <shrothriumrohan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers p2 mid priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants