-
Notifications
You must be signed in to change notification settings - Fork 16
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
Review (wip) #2
Comments
Hello! First of all, thank you, thanks for looking at the repo and sharing your ideas. I think that's a good approach. Would you like to implement it? I recently added integration tests against a geth node running in docker. I found some errors in the typings thanks to those tests but it is a long task, some help would be helpful. Abrazo |
Great, I'll implement the approach I mentioned then, and let you know. |
Any easy ways for us to chat?, I will have several questions while on it |
Telegram? my handle is @menduz |
Agustin,
As promised, I began reading the code, so I can give you a proper review.
First, let me say, awesome work!
I haven't yet used decorators and I like how you used them... well in a sense.
Now to the review...
@inject
and theMethod
is a nice dynamic way of adding behaviour, you are loosing type safety everywhere.You could do something like:
This way eth_getBalance returns a typed function at it will fail at compile time.
I've just done this, but i also saw that
inputAddressFormatter
isn't typed either; but it could.There a few places where parameters or return values are
any
, maybe because everything is a work in progress.For now this is my 2 cents, happy to help if you want code labor.
The text was updated successfully, but these errors were encountered: