Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Gen 672 bug inconsistent winners #26

Merged
merged 6 commits into from
Oct 3, 2023
Merged

Gen 672 bug inconsistent winners #26

merged 6 commits into from
Oct 3, 2023

Conversation

asselstine
Copy link
Contributor

@asselstine asselstine commented Sep 29, 2023 •
edited
Loading

Big changes:

  • Refactored Observation to include uint32 balance again.
  • Observation timestamps are now relative to the PERIOD_OFFSET. this allows them to last an additional 53 years (meaning the twab controller will last approx 136 years after deployment)
  • Instead of trying to handle the timestamp overflow, the system now gracefully degrades. Delegations will all be zero after the current time overflows.

@linear
Copy link

linear bot commented Sep 29, 2023

GEN-672 BUG: inconsistent winners

  1. From underthesea:

    block 109344995  true  vault  0x31515cfc4550d9c83e2d86e8a352886d1364e2d9 add:  0x4d8a650230d9909c1cac2a25ebe4e15debed4e2c  tier  3 index  7
block 109378956  false  vault  0x31515cfc4550d9c83e2d86e8a352886d1364e2d9 add:  0x4d8a650230d9909c1cac2a25ebe4e15debed4e2c  tier  3 index  7

  2. https://discord.com/channels/629836507280048129/630886209278836756/1150518763754635524

This is draw 7, which closed at block 109344984. Sep-09-2023 07:45:45 PM +UTC

Draw 7 end timestamp was 1694286000

  • Block 109344984 timestamp: 1694288745 (period on or end 1694289600)
  • Block 109344995 timestamp: 1694288767 (period on or end 1694289600)
  • Block 109347939 timestamp: 1694294655 (period on or end 1694296800) (went from true to false)
  • Block 109378956 timestamp: 1694356689

The isWinner went from true to false at block 109347939 (timestamp 1694294655)

newest total supply twab observation timestamp 109344984 = 1694285541 (before period on or end)

newest total supply twab observation timestamp 109347939 = 1694294655 (after period on or end)

twab time range: 1694199600 - 1694286000

The V5 draw results were changing.

Draw results at first: GenerationSoftware/pt-v5-draw-results-beta@1ab7124

Draw results after: GenerationSoftware/pt-v5-draw-results-beta@82391e5

It does appear that there is a problem with the Twab Controller, because the twab total supply changed.

@asselstine asselstine force-pushed the gen-672-bug-inconsistent-winners branch from 2f9d736 to b0a9f93 Compare September 29, 2023 21:08
@github-actions
Copy link

LCOV of commit b0a9f93 during 100% Test Coverage #139

Summary coverage rate:
  lines......: 100.0% (239 of 239 lines)
  functions..: 100.0% (53 of 53 functions)
  branches...: no data found

Files changed coverage rate:
                                  |Lines       |Functions  |Branches    
  Filename                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================
  src/TwabController.sol          | 100%    116| 100%    33|    -      0
  src/libraries/ObservationLib.sol| 100%     15| 100%     1|    -      0
  src/libraries/TwabLib.sol       | 100%    108| 100%    19|    -      0

@github-actions
Copy link

LCOV of commit 7be40b2 during 100% Test Coverage #141

Summary coverage rate:
  lines......: 100.0% (239 of 239 lines)
  functions..: 100.0% (53 of 53 functions)
  branches...: no data found

Files changed coverage rate:
                                  |Lines       |Functions  |Branches    
  Filename                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================
  src/TwabController.sol          | 100%    116| 100%    33|    -      0
  src/libraries/ObservationLib.sol| 100%     15| 100%     1|    -      0
  src/libraries/TwabLib.sol       | 100%    108| 100%    19|    -      0

PierrickGT
PierrickGT suggested changes Sep 30, 2023
View reviewed changes
src/TwabController.sol
@@ -102,8 +102,8 @@ contract TwabController {
event ObservationRecorded(
address indexed vault,
address indexed user,
uint112 balance,
uint112 delegateBalance,
uint96 balance,
Copy link
Contributor

Choose a reason for hiding this comment

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

I will need to update this to uint96 in the Vault.

src/TwabController.sol Outdated
if (_timestamp < PERIOD_OFFSET) {
return PERIOD_OFFSET;
}
if ((_timestamp - PERIOD_OFFSET) % PERIOD_LENGTH == 0) {
return _timestamp;
return uint32(_timestamp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to cast it here?
It should pass in the first condition and return PERIOD_OFFSET if _timestamp is greater than PERIOD_OFFSET which is of uint32 type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm; no I don't think we do. I'll do some checks

src/libraries/ObservationLib.sol Outdated
@@ -4,6 +4,8 @@ pragma solidity ^0.8.19;

import "ring-buffer-lib/RingBufferLib.sol";

// import "./OverflowSafeComparatorLib.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed.

src/libraries/ObservationLib.sol Show resolved Hide resolved
src/libraries/TwabLib.sol Outdated Show resolved Hide resolved
test/TwabController.t.sol Outdated Show resolved Hide resolved
test/TwabController.t.sol Outdated Show resolved Hide resolved
test/TwabLib.t.sol Outdated Show resolved Hide resolved
@PierrickGT PierrickGT mentioned this pull request Oct 2, 2023
Merged
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

LCOV of commit 54566d6 during 100% Test Coverage #143

Summary coverage rate:
  lines......: 100.0% (236 of 236 lines)
  functions..: 100.0% (53 of 53 functions)
  branches...: no data found

Files changed coverage rate:
                                  |Lines       |Functions  |Branches    
  Filename                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================
  src/TwabController.sol          | 100%    116| 100%    33|    -      0
  src/libraries/ObservationLib.sol| 100%     15| 100%     1|    -      0
  src/libraries/TwabLib.sol       | 100%    105| 100%    19|    -      0

@asselstine asselstine merged commit 9543316 into main Oct 3, 2023
2 checks passed
@asselstine asselstine deleted the gen-672-bug-inconsistent-winners branch October 3, 2023 19:51
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

LCOV of commit 31d1591 during 100% Test Coverage #145

Summary coverage rate:
  lines......: 100.0% (236 of 236 lines)
  functions..: 100.0% (53 of 53 functions)
  branches...: no data found

Files changed coverage rate:
                                  |Lines       |Functions  |Branches    
  Filename                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================
  src/TwabController.sol          | 100%    116| 100%    33|    -      0
  src/libraries/ObservationLib.sol| 100%     15| 100%     1|    -      0
  src/libraries/TwabLib.sol       | 100%    105| 100%    19|    -      0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PierrickGT PierrickGT PierrickGT requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@asselstine @PierrickGT