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.

Sign up for GitHub

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

Jump to bottom

feat: int in-place testnet creation #4297

Merged
merged 40 commits into from
Sep 3, 2024
Merged

feat: int in-place testnet creation #4297

merged 40 commits into from
Sep 3, 2024

Conversation

likesToEatFish
Copy link
Contributor

close: #4244

This PR includes additional in-place-testnet command

@likesToEatFish
Copy link
Contributor Author

From v28.5.1 and this commit I tried creating a chain called "testchain" and running it with 4 nodes then stopping it. Executing the command and get the expected results:

testchaind in-place-testnet testing-1 cosmosvaloper1w7f3xx7e75p4l7qdym5msqem9rd4dyc4mq79dm --home $HOME/.testchaind/validator1

Chain created from ignite (including node initialization script): testchain

julienrbrt
julienrbrt reviewed Aug 9, 2024
View reviewed changes
Copy link
Member

@julienrbrt julienrbrt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good start, but we are missing a few things:

  • There are too many hard-coded addresses, we should make use of ignite config for that
  • We should create an ignite testnet command to launch this given a path. Meaning the actual appd commands takes many arguments that ignite testnet in-place is passing properly based on the config.yml. Think of it as a wrapper.

ignite/templates/app/files/app/app.go.plush Outdated Show resolved Hide resolved
ignite/templates/app/files/app/app.go.plush Outdated Show resolved Hide resolved
ignite/templates/app/files/app/app.go.plush Outdated Show resolved Hide resolved
ignite/templates/app/files/app/app.go.plush Outdated Show resolved Hide resolved
@likesToEatFish
Copy link
Contributor Author

Thank you for taking the time to review and provide feedback! Your input is invaluable and helps me improve.

Good start, but we are missing a few things:

  • There are too many hard-coded addresses, we should make use of ignite config for that
  • We should create an ignite testnet command to launch this given a path. Meaning the actual appd commands takes many arguments that ignite testnet in-place is passing properly based on the config.yml. Think of it as a wrapper.

@likesToEatFish
Copy link
Contributor Author

likesToEatFish commented Aug 12, 2024 •
edited
Loading

I have made the updates and look forward to receiving your review again @julienrbrt

@likesToEatFish likesToEatFish changed the title int in-place testnet creation feat: int in-place testnet creation Aug 12, 2024
@likesToEatFish
Copy link
Contributor Author

Sorry I'm still lacking, you don't need to review it right away.

@likesToEatFish likesToEatFish marked this pull request as draft August 12, 2024 11:33
@DongLieu DongLieu force-pushed the duong/in-place-testnet branch from f5d1224 to 5d09ef5 Compare August 30, 2024 02:25
@likesToEatFish
Copy link
Contributor Author

I have completed the requirements in this PR and am ready to merge. I am happy to contribute to the project and hope these changes will bring value to the product.
cc: @julienrbrt @Pantani

julienrbrt
julienrbrt reviewed Aug 30, 2024
View reviewed changes
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

Can you verify if nothing break in ignite s chain foo --minimal? Looks like it does: TestGenerateAnAppMinimal

@likesToEatFish
Copy link
Contributor Author

likesToEatFish commented Aug 30, 2024 •
edited
Loading

Can you verify if nothing break in ignite s chain foo --minimal? Looks like it does: TestGenerateAnAppMinimal

For chains created from --minimal, the slashing module is not used, so app.SlashingKeeper does not exist.

This can be solved in two ways:

  • way 1: add Slashing to the chain created from --minimal
  • way 2: remove the new validator's signing information setting in the Slashing module in testnet

I will solve it with way 2 because it comes from this PR.

@julienrbrt
Copy link
Member

Can you verify if nothing break in ignite s chain foo --minimal? Looks like it does: TestGenerateAnAppMinimal

For chains created from --minimal, the slashing module is not used, so app.SlashingKeeper does not exist.

This can be solved in two ways:

  • way 1: add Slashing to the chain created from --minimal
  • way 2: remove the new validator's signing information setting in the Slashing module in testnet

I will solve it with way 2 because it comes from this PR.

We should keep it, we can just have a condition in the template.

@julienrbrt
Copy link
Member

Check this for instance: https://github.com/ignite/cli/blob/main/ignite/templates/app/files/cmd/%7B%7BbinaryNamePrefix%7D%7Dd/cmd/root.go.plush#L80-L90

@likesToEatFish
Copy link
Contributor Author

likesToEatFish commented Aug 31, 2024 •
edited
Loading

Thanks @julienrbrt, looks great.

@julienrbrt julienrbrt requested a review from Pantani September 3, 2024 09:11
@likesToEatFish
Copy link
Contributor Author

I have completed the required work. When can I receive the reward? How will the reward be received?

@julienrbrt
Copy link
Member

I have completed the required work. When can I receive the reward? How will the reward be received?

Yes, congratulations! I will send someone your way to arrange that.

@julienrbrt julienrbrt enabled auto-merge (squash) September 3, 2024 13:05
@likesToEatFish
Copy link
Contributor Author

Thanks! I am glad to have completed the work. For convenience of communication, you can email me at [email protected].

@julienrbrt julienrbrt merged commit f7618d3 into ignite:main Sep 3, 2024
82 of 91 checks passed
mergify bot pushed a commit that referenced this pull request Sep 3, 2024
@likesToEatFish @mergify
feat: int in-place testnet creation (#4297)
fe3c15f 
(cherry picked from commit f7618d3)

# Conflicts:
#	docs/docs/03-CLI-Commands/01-cli-commands.md
#	ignite/pkg/chaincmd/chaincmd.go
@mergify mergify bot mentioned this pull request Sep 3, 2024
Merged
julienrbrt added a commit that referenced this pull request Sep 4, 2024
@mergify @likesToEatFish @julienrbrt
feat: int in-place testnet creation (backport #4297) (#4338)
abbe7f3 
* feat: int in-place testnet creation (#4297)

(cherry picked from commit f7618d3)

# Conflicts:
#	docs/docs/03-CLI-Commands/01-cli-commands.md
#	ignite/pkg/chaincmd/chaincmd.go

* updates

* cl

* re

* consumer handling

---------

Co-authored-by: Duong NV | Decentrio <[email protected]>
Co-authored-by: Julien Robert <[email protected]>
@BrewTestBot BrewTestBot mentioned this pull request Sep 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julienrbrt julienrbrt julienrbrt approved these changes

@toschdev toschdev toschdev approved these changes

@Pantani Pantani Pantani approved these changes

@ilgooz ilgooz Awaiting requested review from ilgooz ilgooz is a code owner

@jeronimoalbi jeronimoalbi Awaiting requested review from jeronimoalbi jeronimoalbi is a code owner

@salmad3 salmad3 Awaiting requested review from salmad3 salmad3 is a code owner

Assignees

@likesToEatFish likesToEatFish

Labels
backport/v28.x.y Backport to v28.x.y component:ci CI/CD workflow and automated jobs. component:cmd component:configs component:docs Documentation additions or improvements. component:packages component:templates type:services Service-related issues.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

INIT: In-Place Testnet Creation
4 participants
@likesToEatFish @julienrbrt @Pantani @toschdev