Enable to set minimum gas price #689
Conversation
because default minGasPrice is 0
|
|
| t.Fatal(err) | ||
| } | ||
|
|
||
| if price.Cmp(minPrice) != 0 { |
There was a problem hiding this comment.
This test passes because the price is greater than the minPrice and thus the Cmp statement is not going to be equal to zero. I'm thinking an additional test where the price does get set to the minimum (as one of the tests but for the maximum gas price does above) would be helpful for full scope of the functionality.
There was a problem hiding this comment.
@vezenovm
Thanks for your review.
NewFloat(0) for the GasMultipler argument of NewConnection in TestConnection_SafeEstimateGasMin, so I think that the gasPrice without considering minGasPrice will be 0. I think.
Therefore, since it is lower than minGasPrice(big.NewFloat(1)), the final price will be 1, and we believe that price.Cmp(minPrice) will be 0, which is the same.
Maybe it is hard to see that GasMultipler is 0, so I think I will make it a local variable, how about that?
There was a problem hiding this comment.
I've added a test, what do you think?
29e0f89
There was a problem hiding this comment.
Apologies for getting back late. I did miss that you made the GasMultiplier 0, and your logic is correct there. I think the local variable addition does help make it more clear as well.
I also think the second test is still good to have as it shows the logic for when the GasMultiplier is 1 which is the more common case.
…feature/min-gas-price
minGasPrice and maxGasPrice is same case
| t.Fatal(err) | ||
| } | ||
|
|
||
| if price.Cmp(minPrice) != 0 { |
There was a problem hiding this comment.
Apologies for getting back late. I did miss that you made the GasMultiplier 0, and your logic is correct there. I think the local variable addition does help make it more clear as well.
I also think the second test is still good to have as it shows the logic for when the GasMultiplier is 1 which is the more common case.
|
|
||
| func TestConnection_SafeEstimateGasSameMin(t *testing.T) { | ||
| // When GasMultipler is set to 1, the gas price is set to 2000000000, so the minPrice is set to the same price. | ||
| minPrice := MaxGasPrice |
There was a problem hiding this comment.
This test looks good, but could you make the maxGasPrice and minPrice different? I know the logic you changed in the estimate gas function isn't very complex I just think it would then be much clearer as to where in the SafeEstimateGas method the gas price was actually returned (https://github.com/ChainSafe/ChainBridge/pull/689/files#diff-349cc0f2c50c806250ed335c780d7a6dd9b7ae1cd01fed7e8cfacfa16bba81f6L160-R169). With minPrice and maxPrice being the same it just seems much less clear. You could just increase the maxGasPrice passed in to NewConnection to anything greater than the minPrice used here. Otherwise I think everything is good though.
There was a problem hiding this comment.
Thank you for your review.
I fixed it 6a6ceeb.
to avoid changing the global MaxPrice
| // In the case of d := c.Add(a, b), since c==d, there is a risk that MaxGasPrice itself will be changed, | ||
| // so the local variable maxGasPrice is defined by big.NewInt(0). | ||
| maxGasPrice := big.NewInt(0) | ||
| // MaxGasPrice is the constant price on the dev network, so we increase it here by 1 to ensure it adjusts | ||
| conn := NewConnection(TestEndpoint, false, AliceKp, log15.Root(), GasLimit, MaxGasPrice.Add(MaxGasPrice, big.NewInt(1)), GasMultipler, "", "") | ||
| maxGasPrice.Add(MaxGasPrice, big.NewInt(1)) | ||
| conn := NewConnection(TestEndpoint, false, AliceKp, log15.Root(), GasLimit, maxGasPrice, MinGasPrice, GasMultipler, "", "") |
There was a problem hiding this comment.
I have modified the way BigInt is added to MaxGasPrice.
The intention of the modification is that d = c.Add(a, b) will result in d == c, which may change the global variable MaxGasPrice from big.NewInt(ethutils.DefaultMaxGasPrice).
|
Task linked: CU-t1kgq6 Trial |
| minPrice, maxGasPrice := big.NewInt(0), big.NewInt(0) | ||
| // When GasMultipler is set to 1, the gas price is set to 2000000000, so the minPrice is set to the same price | ||
| // and maxPrice is made larger than the minPrice by adding one. | ||
| minPrice.Add(MaxGasPrice, big.NewInt(0)) |
There was a problem hiding this comment.
Can't you just set minPrice to MaxGasPrice right away when it is declared and delete this line
There was a problem hiding this comment.
thank you
I fixed it 514ca0b.
Description
Added the ability to set the minimum value for gasPrice in the issue at #529.
The reason why this is necessary is that in some chains there are cases where the minimum value is 470 GWEI etc.
Related Issue Or Context
Close: #529
How Has This Been Tested? Testing details.
I've added test code to make sure minGasPrice works
Also, since the argument of connection.NewConnection has changed, we fixed the tests related to it.
With these fixes, I confirmed that all the
make testpassed.Types of changes
Checklist: