search

simpleledger / Electron-Cash-SLP

Electron Cash for SLP Tokens
https://simpleledger.cash/project/electron-cash-slp-edition/
MIT License
67 stars 53 forks source link

Address format is a class property, creates problems for future SLP merge #54

Open jcramer opened 5 years ago

jcramer commented 5 years ago

When address format is toggled in one wallet it is toggled in all open wallets becuase address format is a static class property (I think, not sure on the correct pythonic way to describe).

This creates a problem with the SLP merge because SLP address format should only be available in the SLP wallets. You can see this problem in action by running the merge-mainline branch with two wallets open (1 standard and 1 bip39-slp). You will notice the effect on a standard wallet when you the SLP wallet is used to toggle into SLP format.

jcramer commented 5 years ago

Here is the code controlling the wallet's address toggle icon: https://github.com/simpleledger/Electron-Cash-SLP/blob/9db747a3b1be5cfd2c41249b25fd5ea7014d1e03/gui/qt/main_window.py#L3505

Here is the method which sets format as a class property: https://github.com/simpleledger/Electron-Cash-SLP/blob/9db747a3b1be5cfd2c41249b25fd5ea7014d1e03/lib/address.py#L492

cculianu commented 5 years ago

When address format is toggled in one wallet it is toggled in all open wallets becuase address format is a static class property

Yeah whatever python calls it -- class attribute, I guess.

Yeah I wouldn't touch this facility in this way though. It makes a lot of sense for everything to be in the same format for the entire app. It really should remain global. As much as that butts heads with what you are trying to do. If we changed that it would break other things... and IMHO make for worse UX in many ways as now the user is looking at potentially 2 address formats at once for EC.

And yes -- this entire scheme needs to be 100% refactored, though, for sure, to not do this for SLP wallets and/or for addresses in the SLP window.

cculianu commented 5 years ago

UPDATE: After discussions on telegram on second thought probably we will need to refactor out the "address format" toggle to not be a global setting but per-wallet. There is no other way that we can easily do what we want without that.

The address format being global saved a lot of coding headaches but it has limitations -- namely this.

I'll look at it this coming week hopefully. I have some ideas. Basically every piece of code that touches "Address.to_ui_string()" will now need a 'context' variable passed to it to tell it what to do...

Ugly but.. it is what it is. We might be able to do things like "push_address_context" or something so that there is a "default" context that is app-global (for the non-SLP wallets) and an optional per-wallet context .. the 'push-wallet-context' would happen in the SLP wallet's code that updates the UI.

Pretty nasty.

I'll try and refactor it to avoid as much nastiness as possible. So much to do....

jcramer commented 5 years ago

@cculianu Maybe there is a simple solution to this problem. Whenever the user clicks on the wallet main_window the wallet Address class format should be updated. Clicking on main_window could trigger a toggle to SLP format if user has clicked on SLP wallet type, and likewise toggle to cashAddr format if user has clicked on non-SLP wallet type.

If you run 3.4.8 (with both wallet types open) you will notice that the address format is updated once the users starts toggling on the toggle button.

The downside of this approach would be that if a user has an SLP wallet open in the foreground and non-SLP in the background then the background wallet will be in SLP address mode, which may be just an annoyance to someone who is picky. But as soon as they would click on the non-SLP wallet the address format would be updated to cashAddr format.

jcramer commented 5 years ago

@fyookball this is something to consider for the current SLP merge.

cculianu commented 5 years ago

I think the solution is to not make it a class property but rather UI code should just explicitly pass down what it wants. It makes it a little bit annoying on the programmer as you'd have to do address.to_string(current_setting) or something .. but that can be search-and-replaced.

Alternatives to that include using thread-local storage and pushing a state onto the threadstore -- things like openGL work this way. You modify a state machine per thread. In our case the state machine has only 2 or 3 states: FMT_CASHADDR, FMT_LEGACY, and for slp, FMT_SLPADDR.

Either of the above would be versatile enough to allow for what we want. However, would be some changes to code.

jcramer commented 5 years ago

@cculianu this sounds good to me, thank you for that explanation and direction. When and how do you think this change should be made? Should it just be changed in the mainline codebase before merging the SLP UI into mainline?

cculianu commented 5 years ago

There's no real need for it now in mainline, so it would feel like "busy work" in that you spend all this time refactoring code and then at the end in the best case -- everything remains exactly the same UX wise! (worst case 1 bug slipped through and you find out 2 weeks later after a release! Ha!).

But perhaps that would be a way to go about it -- prepare the ground for what's to come, sure.

There will be a need when one wants to have SLP wallets, though, for sure. I just saw it as part of the changes required for SLP merge, in my mind. My two cents..

jcramer commented 5 years ago

Sounds good to me, it can be part of the merge process.