-
Notifications
You must be signed in to change notification settings - Fork 46
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
#42: refactor types and utils #86
#42: refactor types and utils #86
Conversation
Signed-off-by: Rohan Shrothrium <[email protected]>
return b, nil | ||
} | ||
|
||
func ReadYamlConfig(path string, o interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These methods are used in few of libraries we import eigensdk in. So let's not remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaah makes sense. Also, can we move these functions to a different module like eigenUtils
because intuitively utils
are for internal use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally all internal stuff would've been in an internal
package but we didn't follow that convention.
I am not sure what the ideal way of doing this. Are you suggesting we can have utils
and eigenUtils
both ? one for internal and one for external?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this might cause breaking changes for the SDK users though.
Honestly, I don't see what these utils functions really abstract away for users. I believe the SDK should be used for its interfaces and the utils like reading config files should be handled by client apps.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea you are right. the only reason I added is that, multiple repository we have use this function and I just wanted to have it once. but I kind of agree it's on client to have these standard utils on their own. they can choose their own way to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shrimalmadhur if you don't mind can you resolve all the comments which do not need any change? I am a bit confused as to what functions should be added back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is removed as well in the initial commit ser!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shrimalmadhur is this good to be merged after we rebase on master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. @RohanShrothrium can you rebase and resolve conflicts? and then we can merge this
@@ -32,15 +32,15 @@ type Operator struct { | |||
} | |||
|
|||
func (o Operator) Validate() error { | |||
if !utils.IsValidEthereumAddress(o.Address) { | |||
if !isValidEthereumAddress(o.Address) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not preserve the Address type as in go-ethereum ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many times the Validate function will be called?
If it will be called many times, think of adding an indication if the address was validated already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a typo at line 22 (addres)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good point. this PR is getting stale. might as well do in new PR. I will make an issue.
|
||
func checkIfUrlIsValid(rawUrl string) error { | ||
// Regular expression to validate URLs | ||
urlPattern := regexp.MustCompile(`^(https?|ftp)://[^\s/$.?#].[^\s]*$`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this regex allows invalid uri
https://a.
https://a.com.
https://1.
It also doesn't allow working with ftps, or other uri schema such as grpc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea this is not perfect. as in we tailored it to our requirement and allowed certain endpoints, but definitely can add more.
Fixes #42 .
Motivation
Clean up
utils
andtypes
modules.Solution
Moved type validations out of utils. Typecasting is moved to the service itself.
Open questions
Right now the
types
module imports the crypto module which should not be the case. Certain types defined in crypto modules are used across other modules of the same hierarchy. Should I accommodate this in the same PR?