Warning if no default account for ens

[alaibe]

Overview

TL;DR
ENS needs default account

Cool Spaceship Picture

[0x-r4bbit]

Left a comment!

[0x-r4bbit]

Shouldn't those be at least instance of Error? e.g.

return callback(new Error(defaultAccountNotSetError))

[andremedeiros]

This is a huge pet peeve of mine too, @PascalPrecht. Each module should define their own errors as constants and pass those around :-(

[0x-r4bbit]

Yes, and I think it's enough to first just upgrade this code to use Error type in the first place. We can then introduce custom errors in the future, in case we need them.

It's just that, the way it looks like to me right now, is that simply because callback() gets called with the message as first parameters, call sides will probably simply expect the first param to be an error a la:

doSomething((err, data) => {
  if (err) {
  // whoops
  } 
  // do something with data
}) 

But even then err is just a string. Semantically it should at least be Error IMO.

For consistency reasons though, I can see why @alaibe went for just using a string. But maybe this is a good opportunity to change to Error in all places (in this module).

[andremedeiros]

Oh, totally. He's keeping consistent, I'm talking about wishful thinking in the future. Something along the lines of:

doSomething((err, data) => {
  if(err) {
    switch(err) {
      case ThisModule.ThisError:
        // ...
      case ThisModule.ThatError:
        // ...
    }
  }
});

[michaelsbradleyjr]

It's a bit of a band-aid, but we do have:

https://github.com/embark-framework/embark/blob/develop/lib/utils/utils.js#L484

If an error passed to a callback is destined for logging, it can be run through that utility function if the type isn't known.

[0x-r4bbit]

I'm all for that! 👌

[0x-r4bbit]

For completeness-sake this (very old) article pretty much sums up the best practices: https://humanwhocodes.com/blog/2009/03/10/the-art-of-throwing-javascript-errors-part-2/

This can be applied to promise-based/async/await APIs as well. Really what it boils down to is that errors are actual errors, so they can be handled correctly if needed.

[alaibe]

I am all up for doing changes you suggest here.
However this is a PR for 3.2 being released very soon.

Also this is a bug fix. I think all these awesome ideas:

• async generators
• await/async
• Error
  Deserve at least a real story.
  Being that close to 3.2 I just wanted to do the minimum vital change. Not being lazy but just wanted to make sure to break the least possible things.

What do you think?

[michaelsbradleyjr]

I agree re: min vital change — I consider/ed the other ideas to be for the future, just thought I'd throw in my 2¢.

[0x-r4bbit]

Absolutely! This can be done in the near future.

I've created #911 so we don't forget :)

[0x-r4bbit]

This LGTM as per discussion. I've created #911 so we can keep track of this. :)