Skip to content
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!: Add support for standard ibc #614

Merged
merged 18 commits into from
Dec 10, 2023
Merged

Conversation

omritoptix
Copy link
Collaborator

PR Standards

Opening a pull request should be able to meet the following requirements


For Author:

  • Targeted PR against correct branch
  • included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@omritoptix omritoptix marked this pull request as ready for review December 6, 2023 11:32
@omritoptix omritoptix requested a review from a team as a code owner December 6, 2023 11:32
@@ -14,7 +14,7 @@ import (

func getRegisterRollappCmd(rollappConfig config.RollappConfig) *exec.Cmd {
cmdArgs := []string{
"tx", "rollapp", "create-rollapp", rollappConfig.RollappID, "stamp1", "genesis-path/1", "3", "3", `{"Addresses":[]}`,
"tx", "rollapp", "create-rollapp", rollappConfig.RollappID, "3", `{"Addresses":[]}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to supply the tokenMetada, so it will be used by the denommetada middleware
I had a branch for it IIRC

cmd/relayer/start/start.go Outdated Show resolved Hide resolved
@omritoptix omritoptix linked an issue Dec 9, 2023 that may be closed by this pull request
if err := utils.ExecBashCmd(createClientsCmd, logFileOption); err != nil {
return ConnectionChannels{}, err
}

//after successful update clients, keep running in the background
updateClientsCmd := r.GetUpdateClientsCmd()
utils.RunCommandEvery(ctx, updateClientsCmd.Path, updateClientsCmd.Args[1:], 20, utils.WithDiscardLogging())
Copy link
Contributor

Choose a reason for hiding this comment

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

how it supposed to work when no clients created?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it loops without success until clients are created. as the commands of clients creation are part of the link creation, and not their own separate command, it becomes more complicated to only run it after client creation. agree it's not ideal and we could query the chains until clients are created but I don't think it's worth the effort currently as the con is simply letting it fail for a few seconds until the clients are created.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah but will it create the "txs" to release the empty blocks?
IIRC we did it for that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea it's fine. the txs for releasing empty blocks are required only after the create clients which are not dependent in that.

return prevVersion.Major < 1
}

func (v *VersionMigratorV1000) PerformMigration(rlpCfg config.RollappConfig) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

No changes required on the rollapp itself? how it's state will change to use the tendermint IBC?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the change to the rollapp itself is in the rollapp binary.

fmt.Println("💈 Updating relayer configuration to match new relayer...")
if err := relayer.UpdateRlyConfigValue(rlpCfg, []string{"chains", rlpCfg.RollappID, "value", "extra-codecs"}, []string{"ethermint"}); err != nil {
fmt.Println("💈 Migrating Rollapp key...")
migrateRollappKeyCmd := exec.Command(consts.Executables.RollappEVM, "keys", "migrate", "--home", rlpCfg.Home+"/relayer/keys/"+rlpCfg.RollappID, "--keyring-backend", "test")
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't rly have this migrate functionally? by rly keys list maybe?
just to avoid the dependency on the rollapp/hub binary

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it doesn't have it.

if err != nil {
fmt.Println(out.String())
fmt.Println("💈 Updating relayer configuration to match new relayer key...")
if err := relayer.UpdateRlyConfigValue(rlpCfg, []string{"chains", rlpCfg.RollappID, "value", "extra-codecs"}, []string{"ethermint"}); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

the ethermint codec won't break something with non-evm rollapp?

@omritoptix omritoptix merged commit 3be9ca6 into main Dec 10, 2023
2 of 3 checks passed
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.

Upgrade roller to support standard IBC
2 participants