-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat (l2): hard fork #591
base: main
Are you sure you want to change the base?
feat (l2): hard fork #591
Conversation
…ension-rdk into tip/rdk_hardfork
x/sequencers/keeper/msg_server.go
Outdated
proto.MessageName(&vestingtypes.PermanentLockedAccount{}): {}, | ||
} | ||
|
||
const BumpSequence = 1_000_000_000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const BumpSequence = 1_000_000_000 | |
const BumpSequence = 10_000_000_000 |
|
||
ctx := sdk.UnwrapSDKContext(goCtx) | ||
|
||
var allErrors error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird, you're making a stack of errors
You should only call Join once
err := m.bumpAccountSequence(ctx, account) | ||
allErrors = errors.Join(allErrors, err) | ||
} else { | ||
// check if it can be handled by something custom | ||
for _, f := range m.accountBumpFilters { | ||
toBump, err := f(accType, account) | ||
if err != nil { | ||
allErrors = errors.Join(allErrors, fmt.Errorf("filter account: %w", err)) | ||
return false | ||
} | ||
if toBump { | ||
err := m.bumpAccountSequence(ctx, account) | ||
allErrors = errors.Join(allErrors, err) | ||
break | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can refac to only call bump once
x/sequencers/keeper/msg_server.go
Outdated
|
||
err := m.upgradeKeeper.ScheduleUpgrade(ctx, upgradetypes.Plan{ | ||
Name: fmt.Sprintf("upgrade-drs-%d", drs.DrsVersion), | ||
Height: ctx.BlockHeight() + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, not sure why the upgrade is scheduled in the next block and not the same..
x/sequencers/keeper/msg_server.go
Outdated
func (m msgServer) updateDrsVersion(ctx sdk.Context, drsVersion uint64) { | ||
oldParams := m.rollapParamsKeeper.GetParams(ctx) | ||
oldParams.DrsVersion = uint32(drsVersion) | ||
m.rollapParamsKeeper.SetParams(ctx, oldParams) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it easier to just leave this to the upgrade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to do it here imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is better here, the upgrade handler I think is responsability of the rollapp, as it kind of case by case and it can't be generic?
+1 height is a blocker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have two separate consensus messages for the upgrade? bump account sequencers + updateDRS? couldnt we do it with a unique upgradedrs message and include the drs update version and bumpsequences logic in the necssary upgrade handler?
x/sequencers/keeper/msg_server.go
Outdated
|
||
err := m.upgradeKeeper.ScheduleUpgrade(ctx, upgradetypes.Plan{ | ||
Name: fmt.Sprintf("upgrade-drs-%d", drs.DrsVersion), | ||
Height: ctx.BlockHeight() + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, not sure why the upgrade is scheduled in the next block and not the same..
x/sequencers/keeper/msg_server.go
Outdated
|
||
m.updateDrsVersion(ctx, drs.DrsVersion) | ||
|
||
err := m.upgradeKeeper.ScheduleUpgrade(ctx, upgradetypes.Plan{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think besides the scheduleupgrade we need to call the SetUpgradeHandler, otherwise it the node stops necause it does not find an upgraded handler for the scheduled upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we dont have a way to setup a generic handler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way it works is that it uses the upgrade name to get the handler, I understand when the fork happens there will be a consensus on the next version of the app to use. And this version should include a handler.
Hey sergi -Not all upgrades will require sequence bump |
ctx := sdk.UnwrapSDKContext(goCtx) | ||
|
||
var allErrors error | ||
m.accountKeeper.IterateAccounts(ctx, func(account authtypes.AccountI) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why u not breaking with error on the first error?
Description
Closes #XXX
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.
PR review checkboxes:
I have...
Unreleased
section inCHANGELOG.md
godoc
commentsSDK Checklist
map
time.Now()
sendCoin
and notSendCoins
Full security checklist here
For Reviewer:
After reviewer approval: