-
Notifications
You must be signed in to change notification settings - Fork 56
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #87 +/- ##
==========================================
- Coverage 93.02% 92.94% -0.08%
==========================================
Files 64 64
Lines 1777 1814 +37
==========================================
+ Hits 1653 1686 +33
- Misses 124 128 +4
Continue to review full report at Codecov.
|
@levity @shkfnly These are all good points. The motivation here initially was just to move the eth gas station fetch from the config in the governance dashboard (or any other app) into the newly expanded gas estimator service. If transactions send a static gas price, that creates a possible situation where someone would have a stuck transaction and retry with the same gas price (i.e. the new transactions would continue to get stuck). Since we've been talking about implementing this tx-retry functionality for a while, it seemed like a good time to take a shot at it. But we could actually address the initial concern by throwing an error, like you suggested, but also resetting the default gas price to
|
src/eth/TransactionObject.js
Outdated
tx = await this._web3Service.getTransaction(this.hash); | ||
if ((tx || {}).blockHash) break; | ||
log('not mined yet'); | ||
await promiseWait(5000); | ||
} | ||
|
||
if (tx && !tx.blockHash) { | ||
this._gasEstimator.transactionSpeed = 'fastest'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm on the fence about this -- on the one hand, this is probably the right thing to do in most cases, and on the other hand, this will change the behavior for all other transactions sent from the same maker instance without telling the user. (unless they have disabled gas price in which case it does nothing)
probably better just to drop it, but to make it clear in the documentation how the user would change gas estimator settings for a live instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@levity what do you think about disabling the gasPrice instead? Granted, that's still a change for all other transactions. But I think with MetaMask, this would cause all other transactions to prompt the user to select a gas price, which is probably the ideal behavior here. But if they're not using MetaMask, it could still be weird...so I could see arguments for either side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still in favor of doing nothing and documenting options for the user. Leaning on the principle of least surprise here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good now -- I would still drop that one change
No description provided.