Update Transfer logic + Better logging #4098
Conversation
|
|
||
| // Re Calculate gas once more to be sure | ||
| if (redo) { | ||
| return this.recalculateGas(false); |
There was a problem hiding this comment.
? We call it twice "to be sure"? There seems to be an underlying issue here if we have to resort to calling twice to get some value back.
There was a problem hiding this comment.
The logic is that : the gas estimation is based on the previous value, which can be modified in the method. Let's say you want to send 5 TOK. You estimate gas with the current value (which might be 0), then change to value accordingly (eg. for full balance you need gas estimation before changing the value). But sending 5 TOK costs more gas than sending 0. Thus you might want to recompute the gas estimate.
From what I understand, this is only needed for full balance transfers ; however I might be wrong and that's why I kept it like this. Any thoughts?
There was a problem hiding this comment.
Ok, the redo makes sense for the full-balance case as you explained. I was worried something goes wrong and we are trying to "be sure" by calling again. I must have missed the redo flag being set only for full transfers, if not the case, it should be.
| @@ -60,6 +62,7 @@ const contractsRoutes = [ | |||
| ]; | |||
|
|
|||
| const settingsRoutes = [ | |||
| { path: 'advanced', component: SettingsAdvanced }, | |||
There was a problem hiding this comment.
This should go under the Parity tab. However, really should not be introducing new flows a day before release at all. (This should wait until 1.6)
There was a problem hiding this comment.
Well I thought this would be more like a hidden feature for very specific edge cases which is debugging unconfirmed issues. Thus not showing the feature, but still easily accessible if needed.
There was a problem hiding this comment.
The issue is that we now have 2 tabs with basically nothing, hence adding the extra stuff (e.g. language selector as an example as well features PR extends the "Parity" tab.)
So let's stick with what we have instead of throwing more tabs at it. (We know it is difficult to combine when things get out of hand, eg. Accounts buttons) We can always split the UI-specific stuff at a later point when it becomes unwieldly.
The point below stands - while this is really just for dev and not in wide use, disable the selection in dev mode. (When we bring in Features as per open PR, we can set when/where we want it visible. Quite happy to have it showing, but not while it covers one small modal and not more.)
There was a problem hiding this comment.
Well it's not really throwing more tabs, since there are no visible links for this section. However, I think that this feature would be quite useful even in production if we add more logging that way. We don't want to pollute the dev console, but having a way to get those logs are sometimes useful (and we don't want to automate this to send them to a log server and whatnot). Nein ?
There was a problem hiding this comment.
100% agreed that we will need this going forward. My concern is adding more bulk right at the end of the release when there is no way it can actually go through proper testing where the actual feature has no real benefit apart from a small slice.
So currently only enable in dev (if (NODE_ENV !=== 'development) return null) and keep the dropdown it in Parity so we can be consistent going forward.
|
|
||
| import layout from '../layout.css'; | ||
|
|
||
| export default class AdvancedSettings extends Component { |
There was a problem hiding this comment.
As per above, needs to go onto pre-existing tab. In addition since there is actually no use for it currently (one single modal), it is a feature that shouldn't be show in NODE_ENV === production.
jacogr
added
A8-looksgood 🦄
|
Changes Unknown when pulling 05e1ad2 on ng-transfer-fix into ** on master**. |
This is a first step to try to solve #4057
The logic in the Transfer Modal has been rewritten, which would hopefully get rid of edge case glitches...
A logging library is introduced : LogLevel. The idea is to be able to log debug information, that would by default not appear. To change the log levels, a user can visit the hidden http://localhost:3000/#/settings/advanced page. Log levels can be chosen per module (right now there's only one).
This would help to fix issues like #4057 without having tons and tons of logs by default.