CityOfZion / neon-wallet

Light wallet for the NEO blockchain
http://neonwallet.com
MIT License
1.03k stars 312 forks source link

dev: GAS claiming improvements #832

Closed brianlenz closed 5 years ago

brianlenz commented 6 years ago

I've seen a couple of issues with claiming GAS in the latest dev branch that didn't appear previously.

Issue 1: Informational Messages Missing

It looks like the GAS claiming process has lost most of the informational messaging. If I claim GAS without a Ledger, I don't see any informational / transactional messages until the last "Claim is successful" message. Previously, it was very useful in that it showed the first transaction to send NEO was submitted and then showed a message indicating that transaction 1 of 2 was processing. Once that send transaction was completed, then it would show the "Claim successful" message. As it stands right now, once you hit the Claim button, you get no indication that a transaction has been submitted aside from the Claim button being disabled.

If you are using a Ledger, you are prompted to sign the first transaction on your Ledger, but then you get no further confirmation. Only once the send transaction has processed does it prompt you to sign the second claim transaction. This isn't an ideal experience as the user isn't given any indication of what's going on and that they need to wait to sign the second transaction. Ultimately, once you are prompted to sign the 2nd transaction, then you do get the "Claim successful" message. So, the process does indeed work, but it's not as informational as it was previously, which may lead to confusion.

Issue 2: Transactions updated before balances

This isn't a huge deal, but it's something I noticed different from previous Neon versions. When you claim GAS and get the "Claim successful" message, you wait for the transaction to clear. In the two tests I've run thus far, both times I saw the transaction appear in the transaction list (following the "Latest blockchain data received"), but the GAS balance was not updated. Ultimately, only upon the next balance refresh did the balance update. I'm guessing this has to do with node sync differences? If they are using different API sources (e.g. Neon wallet DB vs. JSON-RPC), that would explain it.

This isn't that huge of a deal, but it's definitely an odd user experience that the transaction appears but the balance doesn't update at the same time. Mentioning since I don't recall seeing it previously (but I'm not 100% sure it's a completely new issue).

mhuggins commented 6 years ago

I took out some of the redundant messages, but I meant to leave one in that it was processing at the beginning. We should probably add that back in since it sounds like I inadvertently removed it.

Issue 2 I noticed as well, and it's because of load balancing. neonDB seems to update before neoscan does. The solution seems to either be remove load balancing, or find a way to sync those requests against the same API. The former sucks because we lose the benefit of load balancing. The latter is challenging because neon-js manages load balancing, and we'd have to introduce another way of doing it.

Given the choices, I left it as is for now. The following block update corrects any mismatch between the transaction history and the balance/claim button amount.

brianlenz commented 6 years ago

I don't remember there being any redundant messages previously? I thought the process was pretty useful with each of the informational messages displayed each step of the way. It certainly keeps the user informed that the process is still ongoing and what the progress is.

The issue with the load balancing affects more than just the GAS claiming scenario, too. I tested single transactions that would send NEO, GAS, and a NEP5 token. I'd see the transaction display in my transaction list, but only the NEP5 balance was updated initially. Only on a subsequent refresh did the GAS and NEO balance update. It's definitely a bit of an odd experience, especially considering that you can see "partial" updates like this. Perhaps not something that needs to be fixed for this release, but maybe something that should be considered further for the next? It would definitely be a better user experience to have consistency in the UI.

Would simply using the same API endpoint for each "block" of requests address the issue? Load balancing makes sense generally, but I'm not sure we really need load balancing when issuing a series of consecutive requests, especially if it introduces this issue?

mhuggins commented 6 years ago

cc @dvdschwrtz @snowypowers in case you have any thoughts on the load balancing.

mhuggins commented 6 years ago

Restoring messaging to be verbose should be an easy fix if desired.

snowypowers commented 6 years ago

neon-js 3.4.0 has nailed down the TxHistory structure to what I suggested so if you want, you can attempt to use that. That should move almost all our calls to neoscan and stabilise the wallet information for most of the time (unless neoscan starts failing and load balancing starts to kick in).

NEP5 token data is retrieved directly from the NEO node itself so im not surprised if its faster than usual. We are retrieving all these from different sources as there is no 1 source that provides 100% information.

So even without load balancing, we may see a desync between NEP5 data and the rest of the data because of this.

mhuggins commented 6 years ago

The first issue here should be addressed by #834.

jplakali commented 6 years ago

i claimed more than 1 gas in neon wallet but now it shows 0 gas so is there any limit how much gas can be claimed?

comountainclimber commented 5 years ago

Closing this issue for now due to inactivity...