Skip to content

fix: Return nil if negative time elapsed from last block happens#1018

Merged
RafilxTenfen merged 6 commits intomainfrom
rafilx/negative-time-elapsed
Jun 15, 2022
Merged

fix: Return nil if negative time elapsed from last block happens#1018
RafilxTenfen merged 6 commits intomainfrom
rafilx/negative-time-elapsed

Conversation

@RafilxTenfen
Copy link
Copy Markdown
Contributor

@RafilxTenfen RafilxTenfen commented Jun 14, 2022

Description

Implemented Option 3: Modify AccrueAllInterest to treat "negative time elapsed" as "zero time elapsed", aborting the function without modifying the state until an EndBlock where BlockTime > LastInterestTime.

closes: #543


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added appropriate labels to the PR
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@RafilxTenfen RafilxTenfen added T:Bug A regression or bug in the Umee codebase. C:x/leverage labels Jun 14, 2022
@RafilxTenfen RafilxTenfen requested a review from toteki June 14, 2022 15:39
@RafilxTenfen RafilxTenfen self-assigned this Jun 14, 2022
@RafilxTenfen RafilxTenfen changed the title fix: return nil if the current time < privous interest time fix: return nil if the current time < previous interest time Jun 14, 2022
@RafilxTenfen RafilxTenfen changed the title fix: return nil if the current time < previous interest time fix: Return nil if negative time elapsed from last block happens Jun 14, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 14, 2022

Codecov Report

Merging #1018 (dc77869) into main (bd2244f) will decrease coverage by 0.08%.
The diff coverage is 15.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1018      +/-   ##
==========================================
- Coverage   43.96%   43.87%   -0.09%     
==========================================
  Files          64       64              
  Lines        8332     8335       +3     
==========================================
- Hits         3663     3657       -6     
- Misses       4414     4422       +8     
- Partials      255      256       +1     
Impacted Files Coverage Δ
x/leverage/keeper/interest.go 72.97% <15.38%> (-5.65%) ⬇️

"prev", prevInterestTime,
)

return nil
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Log is not enough. I think we should either return some message or keep the error.

Copy link
Copy Markdown
Contributor Author

@RafilxTenfen RafilxTenfen Jun 14, 2022

Choose a reason for hiding this comment

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

What do you think about these options? @robert-zaremba

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO, we should fix the genesis logic rather than handling it here. Let's continue at: #543 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK

@RafilxTenfen RafilxTenfen marked this pull request as ready for review June 14, 2022 16:16
@RafilxTenfen RafilxTenfen requested review from a team as code owners June 14, 2022 16:16
Copy link
Copy Markdown
Contributor

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Don't want to block this PR, but I don't think this is the right way for handling the issue.
Side note: issuing Error log without returning error smells bad.

"prev", prevInterestTime,
)

return nil
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO, we should fix the genesis logic rather than handling it here. Let's continue at: #543 (comment)

@RafilxTenfen
Copy link
Copy Markdown
Contributor Author

@robert-zaremba So, do you think we should change from Error to Warn?

@RafilxTenfen
Copy link
Copy Markdown
Contributor Author

@robert-zaremba So, do you think we should change from Error to Warn?

There is no Warn in Tendermin Logger

Only [Debug,Info, Error]

@toteki
Copy link
Copy Markdown
Contributor

toteki commented Jun 15, 2022

Message can keep Error log level for now. Once tendermint fixes, we can return ErrNegativeTimeElapsed instead of nil, which would bring it back to causing chain halt (which we want in this case).

"prev", prevInterestTime,
)

return nil
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK

@RafilxTenfen RafilxTenfen merged commit ac8cc02 into main Jun 15, 2022
@RafilxTenfen RafilxTenfen deleted the rafilx/negative-time-elapsed branch June 15, 2022 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C:x/leverage T:Bug A regression or bug in the Umee codebase.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chain reaches consensus failure on binary rebuild and restart

4 participants