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.

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

V18 Upgrade Migrations #1084

Merged
merged 41 commits into from
Jan 26, 2024
Merged

V18 Upgrade Migrations #1084

merged 41 commits into from
Jan 26, 2024

Conversation

shellvish
Copy link
Contributor

@shellvish shellvish commented Jan 19, 2024

Migrates User Redemption Records to new format in v18 upgrade

Note: we should test this twice with LocalStride. First, with a slightly stale cache, so we can more accurately gauge what will happen at upgrade time. Second, with the most recent mainnet state (as part of the standard localstride flow).

TODO: Add Unit Tests

Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

The logic looks sound. My main comment is that we should add more unit testing as this is quite a sensitive change.

Copy link
Contributor

@asalzmann asalzmann left a comment

Choose a reason for hiding this comment

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

Overall looks good - need to review in detail again tomorrow. +1 to Riley's comment - should we add more detailed unittests?

@sampocs sampocs force-pushed the v18-upgrade-migrations branch from 5d26eb8 to a1773ab Compare January 25, 2024 18:16
Copy link
Collaborator

@sampocs sampocs left a comment

Choose a reason for hiding this comment

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

thanks for taking this - it looked like a tedious one!

x/stakeibc/keeper/unbonding_records.go Outdated Show resolved Hide resolved
app/upgrades/v18/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v18/upgrades.go Show resolved Hide resolved
app/upgrades/v18/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v18/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v18/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v18/upgrades.go Outdated Show resolved Hide resolved
Co-authored-by: sampocs <sam@stridelabs.co>
app/upgrades/v18/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v18/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v18/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/v18/upgrades.go Show resolved Hide resolved
Copy link
Contributor Author

@shellvish shellvish left a comment

Choose a reason for hiding this comment

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

Looks good to me Sam! One small comment on updating the HZU but otherwise beautiful

app/upgrades/v18/upgrades.go Show resolved Hide resolved
Copy link
Contributor

@asalzmann asalzmann left a comment

Choose a reason for hiding this comment

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

This is looking good, should we also unittest test the upgrade itself? Can also happen in a different PR / someone else could grab it!

app/upgrades/v18/constants.go Show resolved Hide resolved
app/upgrades/v18/constants.go Outdated Show resolved Hide resolved
app/upgrades/v18/upgrades.go Outdated Show resolved Hide resolved
@sampocs sampocs marked this pull request as ready for review January 26, 2024 06:57
NativeTokenAmount: initialNativeAmount2,
})

// Create epoch unbonding records
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we test records in other statuses?

Copy link
Collaborator

Choose a reason for hiding this comment

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

was considering that too, but didn't want the test to get too bloated.

Happy to add it in the morn if you think it makes sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just one other status to make sure the record filtering works

Copy link
Collaborator

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

Unit test looks beautiful!

riley-stride and others added 4 commits January 26, 2024 01:08
Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

re-reviewed with fresh eyes: i'm signed off.

@sampocs sampocs merged commit 57ef6c0 into main Jan 26, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants