Skip to content

Conversation

@anhductn2001
Copy link
Contributor

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 PR targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@anhductn2001 anhductn2001 requested a review from a team as a code owner March 14, 2025 04:30
pterm.Info.Println("Proposal Tx hash: ", txHash)
}

daConfig := dalayer.GetSequencerDAConfig(consts.NodeType.Sequencer)
Copy link
Contributor

Choose a reason for hiding this comment

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

GetSequencerDAConfig creates new config (at least for celestia), overwriting namespace. we should use another function or refactor it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it only does that with Celestia. Actually I haven't added the ability to switch from da to Celestia yet so can we consider it in future PRs? @srene

Copy link
Collaborator

Choose a reason for hiding this comment

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


pterm.Info.Println("updating dymint configuration")

_ = tomlconfig.UpdateFieldInFile(
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 include avail da_config + da_layer and also previous, to allow the transition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain further?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the updated value should be old + new DA, that way the system will know where to transfer from and to where. For example, if the current DA is Celestia and the rollapp is transferred to Avail, then the config should contain both.

	[]string{string("avail", "celestia")},

and

      _ = tomlconfig.UpdateFieldInFile(
	      dymintConfigPath,
	      "da_config",
	      daConfig,
      )

daConfig should contain both, the "current" celestia da config and the future avail da config

Copy link
Collaborator

Choose a reason for hiding this comment

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

this part still doesn't seem to be implemented @anhductn2001

Copy link
Collaborator

@artemijspavlovs artemijspavlovs left a comment

Choose a reason for hiding this comment

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

while #1350 and #1351 were created, please fix them and merge into this PR, the bugs mentioned are important to fix before merging this code into main

Long: ``,
Args: cobra.MaximumNArgs(0),
Run: func(cmd *cobra.Command, args []string) {
envs := []string{"avail"} // TODO: support more DAs
Copy link
Collaborator

Choose a reason for hiding this comment

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

env is not descriptive

Comment on lines +100 to +120
keyName, _ := pterm.DefaultInteractiveTextInput.WithDefaultText("Enter your key name").Show()
keyring, _ := pterm.DefaultInteractiveTextInput.WithDefaultText("Enter your keyring-backend").Show()
title, _ := pterm.DefaultInteractiveTextInput.WithDefaultText("Enter your Proposal Title").Show()
description, _ := pterm.DefaultInteractiveTextInput.WithDefaultText("Enter your Proposal Description").Show()
deposit, _ := pterm.DefaultInteractiveTextInput.WithDefaultText("Enter your Proposal Deposit").Show()
newDAParam := json.RawMessage(fmt.Sprintf(`"%s"`, env))
txHash, err := gov.ParamChangeProposal(home, keyName, keyring,
&utils.ParamChangeProposalJSON{
Title: title,
Description: description,
Changes: utils.ParamChangesJSON{
utils.NewParamChangeJSON("rollappparams", "da", newDAParam),
},
Deposit: deposit + rollappConfig.Denom,
})

if err != nil {
pterm.Error.Println("failed to submit proposal", err)
return
}
pterm.Info.Println("Proposal Tx hash: ", txHash)
Copy link
Collaborator

Choose a reason for hiding this comment

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

over all good, but there are too many inputs for the user to provide, might need ux improvements in the future

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.

4 participants