From 5c1c120d9303db71000dcdb43e3d101ea987e3e2 Mon Sep 17 00:00:00 2001 From: Daniel T <30197399+danwt@users.noreply.github.com> Date: Wed, 27 Nov 2024 12:43:19 +0000 Subject: [PATCH] fix(rollapp): properly disambiguate empty/nil genesis accounts (#1560) --- ibctesting/genesis_bridge_test.go | 10 +-- ibctesting/utils_test.go | 2 +- x/iro/keeper/create_plan.go | 8 +-- .../genesisbridge/genesis_bridge_data.go | 8 +-- x/rollapp/genesisbridge/genesis_transfer.go | 10 +-- x/rollapp/genesisbridge/ibc_module.go | 17 ++--- .../keeper/msg_server_update_rollapp_test.go | 64 +++++++++++++++++++ x/rollapp/keeper/rollapp.go | 7 +- x/rollapp/types/genesis_info.go | 42 ++++++------ x/rollapp/types/message_update_rollapp.go | 27 +++----- 10 files changed, 116 insertions(+), 79 deletions(-) diff --git a/ibctesting/genesis_bridge_test.go b/ibctesting/genesis_bridge_test.go index f288c1d7d..0b974a4bf 100644 --- a/ibctesting/genesis_bridge_test.go +++ b/ibctesting/genesis_bridge_test.go @@ -399,21 +399,17 @@ func (s *transferGenesisSuite) genesisBridgePacket(raGenesisInfo rollapptypes.Ge gb.NativeDenom = meta // add genesis transfer if needed - if raGenesisInfo.GenesisAccounts != nil { - total := math.ZeroInt() - for _, acc := range raGenesisInfo.GenesisAccounts.Accounts { - total = total.Add(acc.Amount) - } + if raGenesisInfo.RequiresTransfer() { gTransfer := transfertypes.NewFungibleTokenPacketData( denom, - total.String(), + raGenesisInfo.GenesisTransferAmount().String(), s.rollappChain().SenderAccount.GetAddress().String(), genesisbridge.HubRecipient, "", ) gb.GenesisTransfer = &gTransfer - gb.GenesisInfo.GenesisAccounts = raGenesisInfo.GenesisAccounts.Accounts + gb.GenesisInfo.GenesisAccounts = raGenesisInfo.Accounts() } bz, err := json.Marshal(gb) diff --git a/ibctesting/utils_test.go b/ibctesting/utils_test.go index eee4ea0f6..01092da43 100644 --- a/ibctesting/utils_test.go +++ b/ibctesting/utils_test.go @@ -174,7 +174,7 @@ func (s *transferGenesisSuite) addGenesisAccounts(genesisAccounts []rollapptypes if rollapp.GenesisInfo.GenesisAccounts == nil { rollapp.GenesisInfo.GenesisAccounts = &rollapptypes.GenesisAccounts{} } - rollapp.GenesisInfo.GenesisAccounts.Accounts = append(rollapp.GenesisInfo.GenesisAccounts.Accounts, genesisAccounts...) + rollapp.GenesisInfo.GenesisAccounts.Accounts = append(rollapp.GenesisInfo.Accounts(), genesisAccounts...) s.hubApp().RollappKeeper.SetRollapp(s.hubCtx(), rollapp) } diff --git a/x/iro/keeper/create_plan.go b/x/iro/keeper/create_plan.go index 5896cb338..f20195c2f 100644 --- a/x/iro/keeper/create_plan.go +++ b/x/iro/keeper/create_plan.go @@ -65,12 +65,8 @@ func (m msgServer) CreatePlan(goCtx context.Context, req *types.MsgCreatePlan) ( return nil, errors.Join(gerrc.ErrFailedPrecondition, types.ErrPlanExists) } - // validate the IRO funding genesis transfer is expected in rollapp's genesis info - if rollapp.GenesisInfo.GenesisAccounts == nil { - return nil, errorsmod.Wrap(gerrc.ErrFailedPrecondition, "genesis accounts not found") - } found = false - for _, gAcc := range rollapp.GenesisInfo.GenesisAccounts.Accounts { + for _, gAcc := range rollapp.GenesisInfo.Accounts() { if gAcc.Address == m.Keeper.GetModuleAccountAddress() { if !gAcc.Amount.Equal(req.AllocatedAmount) { return nil, errorsmod.Wrap(gerrc.ErrFailedPrecondition, "allocated amount mismatch") @@ -80,7 +76,7 @@ func (m msgServer) CreatePlan(goCtx context.Context, req *types.MsgCreatePlan) ( } } if !found { - return nil, errorsmod.Wrap(gerrc.ErrFailedPrecondition, "genesis transfer not found for IRO") + return nil, errorsmod.Wrap(gerrc.ErrFailedPrecondition, "no genesis account for iro module account") } planId, err := m.Keeper.CreatePlan(ctx, req.AllocatedAmount, startTime, preLaunchTime, rollapp, req.BondingCurve, req.IncentivePlanParams) diff --git a/x/rollapp/genesisbridge/genesis_bridge_data.go b/x/rollapp/genesisbridge/genesis_bridge_data.go index 100052911..73a2c987a 100644 --- a/x/rollapp/genesisbridge/genesis_bridge_data.go +++ b/x/rollapp/genesisbridge/genesis_bridge_data.go @@ -56,7 +56,7 @@ func (data GenesisBridgeData) ValidateBasic() error { return nil } -// ValidateBasic performs basic validation checks on the GenesisInfo. +// converts to a native type and validates that func (info GenesisBridgeInfo) ValidateBasic() error { // wrap the genesis info in a GenesisInfo struct, to reuse the validation logic raGenesisInfo := types.GenesisInfo{ @@ -64,11 +64,7 @@ func (info GenesisBridgeInfo) ValidateBasic() error { Bech32Prefix: info.Bech32Prefix, NativeDenom: info.NativeDenom, InitialSupply: info.InitialSupply, - } - if len(info.GenesisAccounts) > 0 { - raGenesisInfo.GenesisAccounts = &types.GenesisAccounts{ - Accounts: info.GenesisAccounts, - } + GenesisAccounts: &types.GenesisAccounts{Accounts: info.GenesisAccounts}, } if !raGenesisInfo.AllSet() { diff --git a/x/rollapp/genesisbridge/genesis_transfer.go b/x/rollapp/genesisbridge/genesis_transfer.go index 0a439a453..5f24ad1bf 100644 --- a/x/rollapp/genesisbridge/genesis_transfer.go +++ b/x/rollapp/genesisbridge/genesis_transfer.go @@ -18,14 +18,10 @@ const ( // HandleGenesisTransfer handles the genesis transfer packet, if present, and expected. // We assume that genesis info is already validated, and the genesis transfer is expected to fulfill the genesis info. func (w IBCModule) handleGenesisTransfer(ctx sdk.Context, ra types.Rollapp, packet channeltypes.Packet, gTransfer *transfertypes.FungibleTokenPacketData) error { - // check if required or expected - required := ra.GenesisInfo.GenesisAccounts != nil - // required but not present - if required && gTransfer == nil { + if ra.GenesisInfo.RequiresTransfer() && gTransfer == nil { return errorsmod.Wrap(gerrc.ErrFailedPrecondition, "genesis transfer required") } - // not required but present - if !required && gTransfer != nil { + if !ra.GenesisInfo.RequiresTransfer() && gTransfer != nil { return errorsmod.Wrap(gerrc.ErrFailedPrecondition, "genesis transfer not expected") } if gTransfer == nil { @@ -44,7 +40,7 @@ func (w IBCModule) handleGenesisTransfer(ctx sdk.Context, ra types.Rollapp, pack } // split the transfer to the genesis accounts - for _, acc := range ra.GenesisInfo.GenesisAccounts.Accounts { + for _, acc := range ra.GenesisInfo.Accounts() { // create a new packet for each account data := transfertypes.NewFungibleTokenPacketData( gTransfer.Denom, diff --git a/x/rollapp/genesisbridge/ibc_module.go b/x/rollapp/genesisbridge/ibc_module.go index 20e9d5814..7e6ad86f9 100644 --- a/x/rollapp/genesisbridge/ibc_module.go +++ b/x/rollapp/genesisbridge/ibc_module.go @@ -170,7 +170,7 @@ func (w IBCModule) ValidateGenesisBridge(ra *types.Rollapp, data GenesisBridgeIn return fmt.Errorf("initial supply mismatch: expected: %v, got: %v", raInfo.InitialSupply, data.InitialSupply) } - err := compareGenesisAccounts(raInfo.GenesisAccounts, data.GenesisAccounts) + err := compareGenesisAccounts(raInfo.Accounts(), data.GenesisAccounts) if err != nil { return errorsmod.Wrap(err, "genesis accounts mismatch") } @@ -178,19 +178,12 @@ func (w IBCModule) ValidateGenesisBridge(ra *types.Rollapp, data GenesisBridgeIn return nil } -func compareGenesisAccounts(raCommitted *types.GenesisAccounts, gbData []types.GenesisAccount) error { - if raCommitted == nil { - if len(gbData) == 0 { - return nil - } - return fmt.Errorf("genesis accounts length mismatch: expected 0, got %d", len(gbData)) - } - - if len(raCommitted.Accounts) != len(gbData) { - return fmt.Errorf("genesis accounts length mismatch: expected %d, got %d", len(raCommitted.Accounts), len(gbData)) +func compareGenesisAccounts(raCommitted []types.GenesisAccount, gbData []types.GenesisAccount) error { + if len(raCommitted) != len(gbData) { + return fmt.Errorf("genesis accounts length mismatch: expected %d, got %d", len(raCommitted), len(gbData)) } - for _, acc := range raCommitted.Accounts { + for _, acc := range raCommitted { found := slices.ContainsFunc(gbData, func(dataAcc types.GenesisAccount) bool { return dataAcc.Address == acc.Address && dataAcc.Amount.Equal(acc.Amount) }) diff --git a/x/rollapp/keeper/msg_server_update_rollapp_test.go b/x/rollapp/keeper/msg_server_update_rollapp_test.go index 57f343ae6..af7613642 100644 --- a/x/rollapp/keeper/msg_server_update_rollapp_test.go +++ b/x/rollapp/keeper/msg_server_update_rollapp_test.go @@ -41,6 +41,7 @@ func (s *RollappTestSuite) TestUpdateRollapp() { Base: "aden", Exponent: 18, }, + GenesisAccounts: &types.GenesisAccounts{}, // Frontend must specify empty type }, }, expError: nil, @@ -269,6 +270,69 @@ func (s *RollappTestSuite) TestUpdateRollapp() { } } +// Update rollapp: fail - try to update genesis checksum when sealed +func (s *RollappTestSuite) TestUpdateRollappRegression() { + const ( + rollappId = "rollapp_1234-1" + initialSequencerAddress = "dym10l6edrf9gjv02um5kp7cmy4zgd26tafz6eqajz" + ) + + goCtx := sdk.WrapSDKContext(s.Ctx) + rollapp := types.Rollapp{ + RollappId: rollappId, + Owner: alice, + InitialSequencer: "", + ChannelId: "", + Launched: true, + VmType: types.Rollapp_EVM, + Metadata: &types.RollappMetadata{ + Website: "", + Description: "", + LogoUrl: "", + Telegram: "", + X: "", + }, + GenesisInfo: types.GenesisInfo{ + GenesisChecksum: "old", + Bech32Prefix: "old", + NativeDenom: types.DenomMetadata{ + Display: "OLD", + Base: "aold", + Exponent: 18, + }, + InitialSupply: sdk.NewInt(1000), + Sealed: true, + GenesisAccounts: &types.GenesisAccounts{ + Accounts: []types.GenesisAccount{ + { + Amount: sdk.NewInt(1000), + Address: initialSequencerAddress, + }, + }, + }, + }, + } + + update := &types.MsgUpdateRollappInformation{ + Owner: alice, + RollappId: rollappId, + InitialSequencer: "", + Metadata: nil, + GenesisInfo: &types.GenesisInfo{ + GenesisAccounts: nil, + }, + } + + s.k().SetRollapp(s.Ctx, rollapp) + + _, err := s.msgServer.UpdateRollappInformation(goCtx, update) + s.ErrorIs(err, types.ErrGenesisInfoSealed) + + expect := len(rollapp.GenesisInfo.Accounts()) + ra := s.k().MustGetRollapp(s.Ctx, rollappId) + s.Require().Equal(expect, len(ra.GenesisInfo.Accounts())) +} + func (s *RollappTestSuite) TestCreateAndUpdateRollapp() { const rollappId = "rollapp_1234-1" diff --git a/x/rollapp/keeper/rollapp.go b/x/rollapp/keeper/rollapp.go index d632e5ddd..c2698c977 100644 --- a/x/rollapp/keeper/rollapp.go +++ b/x/rollapp/keeper/rollapp.go @@ -56,11 +56,8 @@ func (k Keeper) CheckAndUpdateRollappFields(ctx sdk.Context, update *types.MsgUp current.GenesisInfo.InitialSupply = update.GenesisInfo.InitialSupply } - if update.GenesisInfo.GenesisAccounts != nil { - current.GenesisInfo.GenesisAccounts = update.GenesisInfo.GenesisAccounts - } else if current.GenesisInfo.GenesisAccounts != nil { - current.GenesisInfo.GenesisAccounts.Accounts = nil - } + // Frontend always passes new value + current.GenesisInfo.GenesisAccounts = update.GenesisInfo.GenesisAccounts } if update.Metadata != nil && !update.Metadata.IsEmpty() { diff --git a/x/rollapp/types/genesis_info.go b/x/rollapp/types/genesis_info.go index a5c052372..05b9ad925 100644 --- a/x/rollapp/types/genesis_info.go +++ b/x/rollapp/types/genesis_info.go @@ -13,12 +13,21 @@ import ( // We set the maximum amount of genesis accounts to 100 const maxAllowedGenesisAccounts = 100 -func (gi GenesisInfo) GenesisTransferAmount() math.Int { - total := math.ZeroInt() +// Handling should be based on length and contents, not nil status +func (gi GenesisInfo) Accounts() []GenesisAccount { if gi.GenesisAccounts == nil { - return total + return nil } - for _, a := range gi.GenesisAccounts.Accounts { + return gi.GenesisAccounts.Accounts +} + +func (gi GenesisInfo) RequiresTransfer() bool { + return 0 < len(gi.Accounts()) +} + +func (gi GenesisInfo) GenesisTransferAmount() math.Int { + total := math.ZeroInt() + for _, a := range gi.Accounts() { total = total.Add(a.Amount) } return total @@ -52,22 +61,19 @@ func (gi GenesisInfo) Validate() error { return ErrInvalidInitialSupply } - // validate max limit of genesis accounts - if gi.GenesisAccounts != nil { - if len(gi.GenesisAccounts.Accounts) > maxAllowedGenesisAccounts { - return fmt.Errorf("too many genesis accounts: %d", len(gi.GenesisAccounts.Accounts)) - } + if l := len(gi.Accounts()); l > maxAllowedGenesisAccounts { + return fmt.Errorf("too many genesis accounts: %d", l) + } - accountSet := make(map[string]struct{}) - for _, a := range gi.GenesisAccounts.Accounts { - if err := a.ValidateBasic(); err != nil { - return errors.Join(gerrc.ErrInvalidArgument, err) - } - if _, exists := accountSet[a.Address]; exists { - return fmt.Errorf("duplicate genesis account: %s", a.Address) - } - accountSet[a.Address] = struct{}{} + accountSet := make(map[string]struct{}) + for _, a := range gi.Accounts() { + if err := a.ValidateBasic(); err != nil { + return errors.Join(gerrc.ErrInvalidArgument, err) + } + if _, exists := accountSet[a.Address]; exists { + return fmt.Errorf("duplicate genesis account: %s", a.Address) } + accountSet[a.Address] = struct{}{} } return nil } diff --git a/x/rollapp/types/message_update_rollapp.go b/x/rollapp/types/message_update_rollapp.go index 2cc11fe54..fde0eda1e 100644 --- a/x/rollapp/types/message_update_rollapp.go +++ b/x/rollapp/types/message_update_rollapp.go @@ -68,6 +68,8 @@ func (msg *MsgUpdateRollappInformation) ValidateBasic() error { } if msg.GenesisInfo != nil { + // TODO: impl using .Validate() https://github.com/dymensionxyz/dymension/issues/1559 + if len(msg.GenesisInfo.GenesisChecksum) > maxGenesisChecksumLength { return ErrInvalidGenesisChecksum } @@ -78,16 +80,14 @@ func (msg *MsgUpdateRollappInformation) ValidateBasic() error { } } - if msg.GenesisInfo.GenesisAccounts != nil { - // validate max limit of genesis accounts - if len(msg.GenesisInfo.GenesisAccounts.Accounts) > maxAllowedGenesisAccounts { - return fmt.Errorf("too many genesis accounts: %d", len(msg.GenesisInfo.GenesisAccounts.Accounts)) - } + // validate max limit of genesis accounts + if l := len(msg.GenesisInfo.Accounts()); l > maxAllowedGenesisAccounts { + return fmt.Errorf("too many genesis accounts: %d", l) + } - for _, acc := range msg.GenesisInfo.GenesisAccounts.Accounts { - if err := acc.ValidateBasic(); err != nil { - return errorsmod.Wrapf(errors.Join(gerrc.ErrInvalidArgument, err), "genesis account: %v", acc) - } + for _, acc := range msg.GenesisInfo.Accounts() { + if err := acc.ValidateBasic(); err != nil { + return errorsmod.Wrapf(errors.Join(gerrc.ErrInvalidArgument, err), "genesis account: %v", acc) } } } @@ -106,12 +106,5 @@ func (msg *MsgUpdateRollappInformation) UpdatingImmutableValues() bool { } func (msg *MsgUpdateRollappInformation) UpdatingGenesisInfo() bool { - if msg.GenesisInfo == nil { - return false - } - return msg.GenesisInfo.GenesisChecksum != "" || - msg.GenesisInfo.Bech32Prefix != "" || - msg.GenesisInfo.NativeDenom.Base != "" || - !msg.GenesisInfo.InitialSupply.IsNil() || - msg.GenesisInfo.GenesisAccounts != nil + return msg.GenesisInfo != nil }