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

Review (wip) #2

Open
mcortesi opened this issue May 30, 2018 · 4 comments
Open

Review (wip) #2

mcortesi opened this issue May 30, 2018 · 4 comments

Comments

@mcortesi
Copy link
Contributor

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...

  1. I think that even though the @inject and the Method is a nice dynamic way of adding behaviour, you are loosing type safety everywhere.

You could do something like:

type Input<A> = (val: A) => any
type OutputFormatter<X> = (val: any) => X

function newMethod<A1, X>(opts: {
  name: string
  inputs: [Input<A1>]
  output: OutputFormatter<X>
}): (rm: RequestManager) => (a1: A1) => Promise<X>
function newMethod<A1, A2, X>(opts: {
  name: string
  inputs: [Input<A1>, Input<A2>]
  output: OutputFormatter<X>
}): (rm: RequestManager) => (a1: A1, a2: A2) => Promise<X>
function newMethod<A1, A2, A3, X>(opts: {
  name: string
  inputs: [Input<A1>, Input<A2>, Input<A3>]
  output: OutputFormatter<X>
}): (rm: RequestManager) => (a1: A1, a2: A2, a3: A3) => Promise<X>
function newMethod<A1, A2, A3, A4, X>(opts: {
  name: string
  inputs: [Input<A1>, Input<A2>, Input<A3>, Input<A4>]
  output: OutputFormatter<X>
}): (rm: RequestManager) => (a1: A1, a2: A2, a3: A3, a4: A4) => Promise<X>
function newMethod(opts: {
  name: string
  inputs: Input<any>[]
  output: OutputFormatter<any>
}): (rm: RequestManager) => (...args: any[]) => Promise<any> {
  return (rm: RequestManager) => async (...args) => {
    const payload = {
      method: opts.name,
      params: opts.inputs.map((format, i) => format(args[i]))
    }
    const result = await rm.sendAsync(payload)
    return opts.output(result)
  }
}

export const eth_getBalance = newMethod({
  name: 'eth_getBalance',
  inputs: [formatters.inputAddressFormatter, formatters.inputDefaultBlockNumberFormatter],
  output: formatters.outputBigNumberFormatter
})

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.

@menduz
Copy link
Member

menduz commented May 30, 2018

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

@mcortesi
Copy link
Contributor Author

Great, I'll implement the approach I mentioned then, and let you know.

@mcortesi
Copy link
Contributor Author

Any easy ways for us to chat?, I will have several questions while on it

@menduz
Copy link
Member

menduz commented May 31, 2018

Telegram? my handle is @menduz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants