Enable passing a json object for wallet json. #3117
Conversation
|
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? |
|
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. |
|
You are changing the signature from Which is why it requires a minor bump. |
|
minor version bump makes sense, I totally get that. It was just confusing to say "it isn't backwards compatible" since |
|
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
added
on-deck
|
Which subclasses do we have that would be impacted? |
|
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. :) |
|
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? |
|
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). |
|
I just realized this hasn't been merged yet. Any thoughts on an updated timeline? |
This fixes enabling passing a wallet
json objectnot 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 ainvalid JSON walletwhen it actually is.