ethers.js icon indicating copy to clipboard operation
ethers.js copied to clipboard

Enable passing a json object for wallet json.

Open wparad opened this issue 3 years ago • 9 comments

This fixes enabling passing a wallet json object not only a wallet encoded JSON string. Before the error implied there was a problem with the wallet even when there isn't one. We can skip the parsing when it is an object and do the associated wallet checks. This significantly improves the user experience and don't incorrect convey that the wallet is a invalid JSON wallet when it actually is.

wparad avatar Jun 26 '22 16:06 wparad

This is definitely not backwards compatible. I'll need to think about it for a bit whether it will go into the next minor bump. What scenarios do you already have the JSON parsed?

ricmoo avatar Jun 29 '22 20:06 ricmoo

What makes us think this is backwards incompatible? before we were throwing an unnecessary error and now we handle it appropriately?

Seeing as we are using javascript, it's natural to be passing around json objects quite naturally. This happens frequently for instance when we have a list of wallets that are stored in this fashion.

wparad avatar Jun 29 '22 21:06 wparad

You are changing the signature from string to string | object. Changing any signature is no longer backwards compatible.

Which is why it requires a minor bump.

ricmoo avatar Jun 29 '22 21:06 ricmoo

minor version bump makes sense, I totally get that. It was just confusing to say "it isn't backwards compatible" since string | object is backwards compatible to string right? No code can break from making this change, I was pretty sure of that. If wasn't backwards compatible then we would make a major version change, right?

wparad avatar Jun 29 '22 21:06 wparad

Right. Perhaps forwards-compatible is the right phrase. Yes, backwards breaking changes are major version changes.

The main point where a signature change matters is in methods, where widening a type has implications for existing sub-classes (as the sub-class may have the narrower type, and therefore becomes incompatible with its super type).

ricmoo avatar Aug 17 '22 22:08 ricmoo

Which subclasses do we have that would be impacted?

wparad avatar Aug 18 '22 05:08 wparad

In this case none, I just meant in general, widening a method signature type breaks backwards compatibility. Widening a function signature type should be safe. :)

ricmoo avatar Aug 18 '22 05:08 ricmoo

Do you mean widening a function signature parameter type versus a function's signature return type?

If you are a making a different distinction, I'm not getting it, a method and a function mean the same thing.

But it any case that's academic, right 😉 , we are saying this is good to be merged, I'm guessing. Or is there something else you would like me to do here?

wparad avatar Aug 18 '22 05:08 wparad

Yes, largely academic. Although, I use method to refer specifically to an objects operation (where the called scope has a this) vs a function which is just a bunch of code sitting out in space.

I was planning to merge it earlier today for the 5.7, but I’m already making a great deal of changes, testing and whatnot and need to get this out sooner than later, so I may push this to 5.8 or just include it in v6. At this point I want to spend more time getting v6 polished off, than risk breaking anything and keep v5 mostly maintained rather than extending functionality. But v5.7 is needed since there are merge changes necessary (and Görli is already there).

ricmoo avatar Aug 18 '22 05:08 ricmoo

I just realized this hasn't been merged yet. Any thoughts on an updated timeline?

wparad avatar Jan 15 '23 17:01 wparad