#42: refactor types and utils #86
Conversation
Signed-off-by: Rohan Shrothrium <shrothriumrohan@gmail.com>
| return b, nil | ||
| } | ||
|
|
||
| func ReadYamlConfig(path string, o interface{}) error { |
There was a problem hiding this comment.
These methods are used in few of libraries we import eigensdk in. So let's not remove
There was a problem hiding this comment.
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.
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.
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.
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.
@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.
This function is removed as well in the initial commit ser!
There was a problem hiding this comment.
@shrimalmadhur is this good to be merged after we rebase on master?
There was a problem hiding this comment.
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.
Why not preserve the Address type as in go-ethereum ?
There was a problem hiding this comment.
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.
there is a typo at line 22 (addres)
There was a problem hiding this comment.
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.
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.
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
utilsandtypesmodules.Solution
Moved type validations out of utils. Typecasting is moved to the service itself.
Open questions
Right now the
typesmodule 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?