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

configure mainnet using build tags #700

Merged
merged 8 commits into from Jun 26, 2020
Merged

configure mainnet using build tags #700

merged 8 commits into from Jun 26, 2020

Conversation

akhilkumarpilli
Copy link
Contributor

@akhilkumarpilli akhilkumarpilli commented Jun 23, 2020

Fixes #683

@codecov
Copy link

codecov bot commented Jun 23, 2020

Codecov Report

Merging #700 into master will increase coverage by 0.40%.
The diff coverage is 95.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #700      +/-   ##
==========================================
+ Coverage   30.41%   30.82%   +0.40%     
==========================================
  Files         147      149       +2     
  Lines        7020     7063      +43     
==========================================
+ Hits         2135     2177      +42     
- Misses       4776     4778       +2     
+ Partials      109      108       -1     
Impacted Files Coverage Δ
cmd/akashctl/configure.go 0.00% <0.00%> (ø)
cmd/akashctl/main.go 0.00% <0.00%> (ø)
app/app.go 93.75% <100.00%> (-0.40%) ⬇️
app/app_configure.go 100.00% <100.00%> (ø)
app/config.go 93.33% <100.00%> (+0.22%) ⬆️
validation/resources.go 51.66% <0.00%> (+5.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7de0f0...57d3e89. Read the comment docs.

@anilcse anilcse marked this pull request as ready for review June 23, 2020 17:17
@anilcse
Copy link
Contributor

anilcse commented Jun 23, 2020

@akhilkumarpilli looks good, file namings are bit odd. Lets think of different namings. How about config/default.go and config/mainnet.go?

Copy link
Contributor

@boz boz left a comment

Choose a reason for hiding this comment

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

Very cool, thank you.

I left some comments inline.

Can you see if there's a way that we can reduce the amount of common code that is duplicated? These areas are really tedious and easy to do wrong.

.github/workflows/integration.yaml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
app/app.go Show resolved Hide resolved
),

provider.NewAppModule(app.keeper.provider, app.keeper.bank, app.keeper.market),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here. for instance, does ModuleManager have an AppendModule() function or something that can be used in the tag-specific functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not able to find a way to append modules in ModuleManager. Tried to reduce the amount of duplicate code and left with these.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, how about

app.mm = module.NewManager(
  genutil.NewModule(),
  // ...
  app.createAkashModules()...,
)
// ...

func (app *App) createAkashModules() []sdk.AppModule {
// ...
}

or even

module.NewManager(app.createAppModules()...)

//...
func (app *App) createAppModules() []sdk.AppModules {
  return append(app.createCoreAppModules(),app.createAkashAppModules()...)
}

func (app *App) createCoreModulesBase() []sdk.AppModules {
  return []sdk.AppModule{
  }
}
// mainnet version:
func (app *App) createAkashAppModules() []sdk.AppModule {
  return []sdk.AppModule{}
}

// !mainnet version
func (app *App) createAkashAppModules() []sdk.AppModule {
  return []sdk.AppModule{
    deployment.NewAppModule(),
    // ...
  }
}

@akhilkumarpilli
Copy link
Contributor Author

@akhilkumarpilli looks good, file namings are bit odd. Lets think of different namings. How about config/default.go and config/mainnet.go?

If we rename files to config/default.go and config/mainnet.go, then package will change. Then I think it will be quite difficult to understand code.

Copy link
Contributor

@boz boz left a comment

Choose a reason for hiding this comment

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

nice work, thank you.

we have to reduce the amount of duplicated setup code if we're going to go this route, tho. I put some ideas in comments inline.

),

provider.NewAppModule(app.keeper.provider, app.keeper.bank, app.keeper.market),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, how about

app.mm = module.NewManager(
  genutil.NewModule(),
  // ...
  app.createAkashModules()...,
)
// ...

func (app *App) createAkashModules() []sdk.AppModule {
// ...
}

or even

module.NewManager(app.createAppModules()...)

//...
func (app *App) createAppModules() []sdk.AppModules {
  return append(app.createCoreAppModules(),app.createAkashAppModules()...)
}

func (app *App) createCoreModulesBase() []sdk.AppModules {
  return []sdk.AppModule{
  }
}
// mainnet version:
func (app *App) createAkashAppModules() []sdk.AppModule {
  return []sdk.AppModule{}
}

// !mainnet version
func (app *App) createAkashAppModules() []sdk.AppModule {
  return []sdk.AppModule{
    deployment.NewAppModule(),
    // ...
  }
}

deployment.ModuleName,
provider.ModuleName,
market.ModuleName,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here.

deployment.NewAppModuleSimulation(app.keeper.deployment, app.keeper.acct),
market.NewAppModuleSimulation(app.keeper.market, app.keeper.acct, app.keeper.deployment,
app.keeper.provider, app.keeper.bank),
provider.NewAppModuleSimulation(app.keeper.provider, app.keeper.acct),
Copy link
Contributor

Choose a reason for hiding this comment

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

and here

Copy link
Contributor

@boz boz left a comment

Choose a reason for hiding this comment

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

Love it, great work.

@boz
Copy link
Contributor

boz commented Jun 26, 2020

sorry @akhilkumarpilli I forgot to merge this.

@boz boz merged commit 50d68fe into master Jun 26, 2020
@boz boz deleted the akhil/mainnet-build-tags branch June 26, 2020 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants