[DO NOT MERGE] v0.52.x integration CI tests #2
Conversation
- still some pending fixes.
Fix all errors in app.go - Add RegisterServices to Configurator, satisfying 0.52 interface. Include acceptedMessages logic. - Update module manager to sync with 0.52 implementation, copy over code where appropriate. - Refactor x/blobstream, signal to align with 0.52 APIs.
- lots of wrapping, seems OK
… keeper appversion
julienrbrt
changed the title
…tore keys, inifine recursion, use sdk lib
this is now being tracked on linear, keeping this alive may be confusing
There was a problem hiding this comment.
wow, this is a ton of work! awesome job @julienrbrt and @kocubinski
mainly just outlined some todos and questions
as discussed during the call, I'm okay with merging this to a feature branch and addressing things after. this PR is already huge and we still need to change quite a few things after the multiplexer is added
| func hasForbiddenParams(msg sdk.Msg, typeURL string, forbiddenParams []string) bool { | ||
| // unmarshal msg to go struct | ||
| // check if any forbidden param is present and different from the default value | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
[todo]
if we don't want to handle everything in this PR I think that's fine! If that's the case, perhaps we treat this fork like a feature branch (or retarget one) and open issues?
| // ICA Controller keeper | ||
| app.ICAControllerKeeper = icacontrollerkeeper.NewKeeper( |
There was a problem hiding this comment.
[question]
iirc, we're keeping legacy ibc and eureaka, and ica will only work on legacy for now?
There was a problem hiding this comment.
correct. ica only works with legacy "classic" ibc
| // TODO: how should this test function in 0.52? | ||
| // { | ||
| // name: "create continuous vesting account with a start time in the future", | ||
| // msgFunc: func() (msgs []sdk.Msg, signer string) { | ||
| // _, _, err := cctx.Keyring.NewMnemonic(vestAccName, keyring.English, "", "", hd.Secp256k1) | ||
| // require.NoError(t, err) | ||
| // sendingAccAddr := testfactory.GetAddress(cctx.Keyring, sendAccName) | ||
| // vestAccAddr := testfactory.GetAddress(cctx.Keyring, vestAccName) | ||
| // msg := vestingtypes.NewMsgCreateVestingAccount( | ||
| // sendingAccAddr, | ||
| // vestAccAddr, | ||
| // sdk.NewCoins(sdk.NewCoin(app.BondDenom, math.NewInt(1000000))), | ||
| // time.Now().Unix(), | ||
| // time.Now().Add(time.Second*100).Unix(), | ||
| // false, | ||
| // ) | ||
| // return []sdk.Msg{msg}, sendAccName | ||
| // }, | ||
| // }, |
There was a problem hiding this comment.
[question]
is it possible to keep the existing vesting module or maintain all of its functionality? all of the legal / ops is built around it
There was a problem hiding this comment.
The vesting module has been entirely removed from 0.52, only its account type are there, so vesting account keep working but you cannot create new ones.
If you want to keep it, we can copy it entirely to celestia-app (and make sure the msg type url stays the same). The idea was that people use x/accounts instead. I can imagine if you have process built around it, it is easier to keep it.
| { | ||
| name: "create continuous vesting account with a start time in the future", | ||
| msgFunc: func() (msgs []sdk.Msg, signer string) { | ||
| vestAccName := "vesting" | ||
| _, _, err := s.cctx.Keyring.NewMnemonic(vestAccName, keyring.English, "", "", hd.Secp256k1) | ||
| require.NoError(t, err) | ||
| sendAcc := s.unusedAccount() | ||
| sendingAccAddr := testfactory.GetAddress(s.cctx.Keyring, sendAcc) | ||
| vestAccAddr := testfactory.GetAddress(s.cctx.Keyring, vestAccName) | ||
| msg := vestingtypes.NewMsgCreateVestingAccount( | ||
| sendingAccAddr, | ||
| vestAccAddr, | ||
| sdk.NewCoins(sdk.NewCoin(app.BondDenom, sdk.NewInt(1000000))), | ||
| time.Now().Add(time.Hour).Unix(), | ||
| time.Now().Add(time.Hour*2).Unix(), | ||
| false, | ||
| ) | ||
| return []sdk.Msg{msg}, sendAcc | ||
| }, | ||
| expectedCode: abci.CodeTypeOK, | ||
| }, |
There was a problem hiding this comment.
If we bring back the vesting module we would be able to bring this back yes.
| @@ -15,6 +18,8 @@ service Msg { | |||
|
|
|||
| // MsgPayForBlobs pays for the inclusion of a blob in the block. | |||
| message MsgPayForBlobs { | |||
| option (cosmos.msg.v1.signer) = "signer"; | |||
There was a problem hiding this comment.
[question]
where can I find out what this does?
There was a problem hiding this comment.
| if b.manifest.PushTrace { | ||
| log.Println("reading trace push config") | ||
| if pushConfig, err := trace.GetPushConfigFromEnv(); err == nil { | ||
| log.Print("Setting up trace push config") | ||
| envVars := map[string]string{ | ||
| trace.PushBucketName: pushConfig.BucketName, | ||
| trace.PushRegion: pushConfig.Region, | ||
| trace.PushAccessKey: pushConfig.AccessKey, | ||
| trace.PushKey: pushConfig.SecretKey, | ||
| trace.PushDelay: fmt.Sprintf("%d", pushConfig.PushDelay), | ||
| } | ||
| for _, node := range b.Nodes() { | ||
| for key, value := range envVars { | ||
| if err = node.Instance.Build().SetEnvironmentVariable(key, value); err != nil { | ||
| return fmt.Errorf("failed to set %s: %v", key, err) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // TODO: waiting on "github.com/cometbft/cometbft/pkg/trace" to be implemented | ||
| // if b.manifest.PushTrace { | ||
| // log.Println("reading trace push config") | ||
| // if pushConfig, err := trace.GetPushConfigFromEnv(); err == nil { | ||
| // log.Print("Setting up trace push config") | ||
| // envVars := map[string]string{ | ||
| // trace.PushBucketName: pushConfig.BucketName, | ||
| // trace.PushRegion: pushConfig.Region, | ||
| // trace.PushAccessKey: pushConfig.AccessKey, | ||
| // trace.PushKey: pushConfig.SecretKey, | ||
| // trace.PushDelay: fmt.Sprintf("%d", pushConfig.PushDelay), | ||
| // } | ||
| // for _, node := range b.Nodes() { | ||
| // for key, value := range envVars { | ||
| // if err = node.Instance.Build().SetEnvironmentVariable(key, value); err != nil { | ||
| // return fmt.Errorf("failed to set %s: %v", key, err) | ||
| // } | ||
| // } | ||
| // } |
There was a problem hiding this comment.
I think we can remove this as we don't use traces in knuu tests atm really. the files should be uploaded by knuu automatically to some s3 bucket if it does need to occur
This PR is only meant to see CI in the fork