Skip to content
This repository has been archived by the owner on Dec 5, 2021. It is now read-only.

Allow previous gas limit #462

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from
Draft

Allow previous gas limit #462

wants to merge 10 commits into from

Conversation

boyuan-chen
Copy link

No description provided.

@boyuan-chen boyuan-chen reopened this Sep 15, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2021

Codecov Report

Merging #462 (31c61e3) into develop (a1662c3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #462   +/-   ##
========================================
  Coverage    76.10%   76.10%           
========================================
  Files           81       81           
  Lines         3038     3038           
  Branches       469      469           
========================================
  Hits          2312     2312           
  Misses         726      726           
Flag Coverage Δ
batch-submitter 59.59% <ø> (ø)
contracts 86.05% <ø> (ø)
core-utils 68.16% <ø> (ø)
data-transport-layer 37.68% <ø> (ø)
message-relayer 72.22% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1662c3...31c61e3. Read the comment docs.

@boyuan-chen boyuan-chen reopened this Sep 15, 2021
@@ -135,7 +140,7 @@ func PaysEnough(opts *PaysEnoughOpts) error {
overpaying := new(big.Int).Sub(opts.UserFee, opts.ExpectedFee)
threshold := mulByFloat(opts.ExpectedFee, opts.ThresholdUp)
// if overpaying > threshold, return error
if overpaying.Cmp(threshold) == 1 {
if overpaying.Cmp(threshold) == 1 && opts.UserFee.Cmp(previousFee) == 1 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we confirm the logic here? Sure we want &&?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the logic here should be "are you overpaying according to both the current and previous limits?". Calculate "overpaying" as shown, calculate "overpaying_2" as Sub(opts.UserFee, opts.previousFee), calculate "threshold_2" by scaling up previousFee, then add "&& overpaying_2.cmp(threshold_2) == 1".

Copy link

@CAPtheorem CAPtheorem Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See most recent commit - simplified the logic per your comment

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants