Skip to content

feat(upgrade): upgrade handler for v1.2#511

Merged
limengformal merged 6 commits intorelease/1.2from
jdub/upgrade-handler-1.2
Mar 18, 2025
Merged

feat(upgrade): upgrade handler for v1.2#511
limengformal merged 6 commits intorelease/1.2from
jdub/upgrade-handler-1.2

Conversation

@jdubpark
Copy link
Contributor

upgrade handler to set new params for validator fallback (#484) in v1.2

issue: none

@jdubpark jdubpark changed the base branch from main to release/1.2 March 12, 2025 19:59
@jdubpark jdubpark changed the title feat: upgrade handler for v1.2 feat: upgrade handler for v1_2 Mar 12, 2025
@jdubpark jdubpark changed the title feat: upgrade handler for v1_2 feat(upgrade): upgrade handler for v1.2 Mar 12, 2025
)

const (
UpgradeName = "v1.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

let's put v1.2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in eff22af

@@ -0,0 +1,49 @@
//nolint:revive,stylecheck // versioning
package v_1_2
Copy link
Contributor

Choose a reason for hiding this comment

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

v1_2_0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in eff22af

Comment on lines 19 to 21
AeneidUpgradeHeight = 10_000_000
// StoryUpgradeHeight defines the block height at which the upgrade is triggered on Story.
StoryUpgradeHeight = 10_000_000
Copy link
Contributor

Choose a reason for hiding this comment

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

Since single binary upgrade is not ready yet, do we want to hardcode the upgrade height?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're correct, we need to use the normal upgrade handler (I copied constants.go from virgil upgrade). Fixed in eff22af

StoreUpgrades: storetypes.StoreUpgrades{},
}

var Fork = upgrades.Fork{
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Do we want to use upgrade module only since single binary upgrade is not ready yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in eff22af

Copy link
Contributor

@0xHansLee 0xHansLee left a comment

Choose a reason for hiding this comment

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

Need to add Upgrade and Fork to app/upgrades.go


// Initial parameters.
InitialRefundFeeBps = 100 // 1%
InitialRefundPeriod = 7 * 24 * time.Hour // 7 days
Copy link
Contributor

Choose a reason for hiding this comment

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

qq. We want to set 7 days for initial refund period, not 1 day?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, I recall we talked about the parameter value. Changed to 1 day here d2404ff, please confirm that 1 day is the intended duration @gezerui0124 @edisonz0718

Copy link
Contributor

Choose a reason for hiding this comment

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

And please check the refundFeeBps as well 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

1 day + 1% LGTM

Copy link
Contributor

@0xHansLee 0xHansLee left a comment

Choose a reason for hiding this comment

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

Please add Fork to app/upgrades/go#L23 as well. And also need to set upgrade height in scheduleForkUpgrade depending on the network and set schedule accordingly.

@jdubpark
Copy link
Contributor Author

Please add Fork to app/upgrades/go#L23 as well. And also need to set upgrade height in scheduleForkUpgrade depending on the network and set schedule accordingly.

We plan to schedule the upgrade through the UpgradeEntrypoint contract, which will use the upgrade module to set the upgrade handler with the upgrade details, including the height.

StoreUpgrades: storetypes.StoreUpgrades{},
}

var Fork = upgrades.Fork{
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Fork used anywhere for the upgrade?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not used actually, removed in 71c1d91

@jdubpark jdubpark requested a review from limengformal March 18, 2025 08:18

// Initial parameters.
InitialRefundFeeBps uint32 = 100 // 1%
InitialRefundPeriod time.Duration = 1 * 24 * time.Hour // 1 days
Copy link
Contributor

Choose a reason for hiding this comment

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

24 * time.Hour

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@limengformal limengformal merged commit b2cc7d0 into release/1.2 Mar 18, 2025
5 checks passed
@limengformal limengformal deleted the jdub/upgrade-handler-1.2 branch March 18, 2025 19:17
0xHansLee pushed a commit that referenced this pull request Apr 11, 2025
upgrade handler to set new params for validator fallback (#484) in v1.2

issue: none
0xHansLee pushed a commit that referenced this pull request Apr 16, 2025
upgrade handler to set new params for validator fallback (#484) in v1.2

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants