search

persistenceOne / pstake-native

Tendermint Implementation of pStake
Apache License 2.0
21 stars 11 forks source link

RegisterLegacyAminoCodec uses names more that 39 chars in length #430

Closed xlab closed 1 year ago

xlab commented 1 year ago

From this piece: 

// RegisterLegacyAminoCodec registers the necessary x/lscosmos interfaces and concrete types
// on the provided LegacyAmino codec. These types are used for Amino JSON serialization.
func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
    cdc.RegisterConcrete(&MinDepositAndFeeChangeProposal{}, "cosmos/MinDepositAndFeeChangeProposal", nil)
    cdc.RegisterConcrete(&PstakeFeeAddressChangeProposal{}, "cosmos/PstakeFeeAddressChangeProposal", nil)
    cdc.RegisterConcrete(&AllowListedValidatorSetChangeProposal{}, "cosmos/AllowListedValidatorSetChangeProposal", nil)
    cdc.RegisterConcrete(&MsgLiquidStake{}, "cosmos/MsgLiquidStake", nil)
    cdc.RegisterConcrete(&MsgLiquidUnstake{}, "cosmos/MsgLiquidUnstake", nil)
    cdc.RegisterConcrete(&MsgRedeem{}, "cosmos/MsgRedeem", nil)
    cdc.RegisterConcrete(&MsgClaim{}, "cosmos/MsgClaim", nil)
    cdc.RegisterConcrete(&MsgRecreateICA{}, "cosmos/MsgRecreateICA", nil)
    cdc.RegisterConcrete(&MsgJumpStart{}, "cosmos/MsgJumpStart", nil)
    cdc.RegisterConcrete(&MsgChangeModuleState{}, "cosmos/MsgChangeModuleState", nil)
    //cdc.RegisterConcrete(&MinDepositAndFeeChangeProposal{}, "pstake/MinDepositAndFeeChange", nil)
    //cdc.RegisterConcrete(&PstakeFeeAddressChangeProposal{}, "pstake/PstakeFeeAddressChange", nil)
    //cdc.RegisterConcrete(&AllowListedValidatorSetChangeProposal{}, "pstake/AllowListedValidatorSetChange", nil)
}

1) In Cosmos SDK v46, cdc.RegisterConcrete has been deprecated in favour of legacy.RegisterAminoMsg, see the reason there https://github.com/cosmos/cosmos-sdk/issues/10870 - RegisterAminoMsg first checks that the msgName is <40 chars (else this would break ledger nano signing)

2) the name cosmos/AllowListedValidatorSetChangeProposal is >40 chars

If left unattended, some messages will fail to sign using certain Ledger devices, as well as maybe other devices.

Also, I'd propose to change prefix cosmos/ to something else. We have LSM in works, so my idea to differentiate is lsnative vs lscosmos, maybe this module can have prefix ls/ to save on the space. Discussion is open.

puneet2019 commented 1 year ago

all deprecated/ removed.