Skip to content

fix(x/auth/vesting): panic on overflowing & negative EndTimes for PeriodicVestingAccount#16733

Merged
julienrbrt merged 1 commit into
mainfrom
x-auth-vesting-types-catch-negative-vestingPeriods+overflows
Jun 28, 2023
Merged

fix(x/auth/vesting): panic on overflowing & negative EndTimes for PeriodicVestingAccount#16733
julienrbrt merged 1 commit into
mainfrom
x-auth-vesting-types-catch-negative-vestingPeriods+overflows

Conversation

@odeke-em
Copy link
Copy Markdown
Contributor

Caught in an audit, this change panics when a PeriodicVestingAccount's constituent period lengths are negative, or if the total .EndTime has overflown.

/cc @elias-orijtech

@odeke-em odeke-em requested a review from a team June 28, 2023 00:39
@ghost ghost self-requested a review June 28, 2023 00:39
@ghost ghost requested review from alexanderbez and julienrbrt and removed request for a team June 28, 2023 00:39
@github-actions

This comment has been minimized.

@odeke-em odeke-em force-pushed the x-auth-vesting-types-catch-negative-vestingPeriods+overflows branch from 375d819 to d08d617 Compare June 28, 2023 00:40
@julienrbrt julienrbrt changed the title fix: x/auth/vesting: panic on overflowing & negative EndTimes for PeriodicVestingAccount fix(x/auth/vesting): panic on overflowing & negative EndTimes for PeriodicVestingAccount Jun 28, 2023
Comment thread CHANGELOG.md Outdated
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.

not: there is already a bugfix section below, can you add it there?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you @julienrbrt, updated!

Comment thread x/auth/vesting/types/vesting_account.go Outdated
Copy link
Copy Markdown
Contributor

@julienrbrt julienrbrt Jun 28, 2023

Choose a reason for hiding this comment

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

Should this check be in the Validate function of a base vesting account and here we can call validate?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am skeptical of moving it there, for starters and for brevity can we keep it as is here, then later on refactor it?

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.

Sure!

…iodicVestingAccount

Caught in an audit, this change panics when a PeriodicVestingAccount's
constituent period lengths are negative, or if the total .EndTime has
overflown.
@odeke-em odeke-em force-pushed the x-auth-vesting-types-catch-negative-vestingPeriods+overflows branch from d08d617 to 771cff2 Compare June 28, 2023 08:35
@odeke-em odeke-em requested a review from julienrbrt June 28, 2023 08:36
@julienrbrt julienrbrt enabled auto-merge June 28, 2023 08:46
@julienrbrt julienrbrt added this pull request to the merge queue Jun 28, 2023
Merged via the queue into main with commit d90abbe Jun 28, 2023
@julienrbrt julienrbrt deleted the x-auth-vesting-types-catch-negative-vestingPeriods+overflows branch June 28, 2023 09:19
mergify Bot pushed a commit that referenced this pull request Jun 28, 2023
…iodicVestingAccount (#16733)

(cherry picked from commit d90abbe)

# Conflicts:
#	CHANGELOG.md
julienrbrt added a commit that referenced this pull request Jun 28, 2023
…iodicVestingAccount (backport #16733) (#16735)

Co-authored-by: Emmanuel T Odeke <emmanuel@orijtech.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
@faddat faddat mentioned this pull request Nov 8, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants