Codebase migration refactor #464

fedekunze · 2020-03-19T17:32:54Z

closes #457

Description

Motivation and Context

I have raised an issue to propose this change (required)
My issue has received approval from the maintainers or lead with the design/approved label

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

boz

What is the motivation for these changes?

boz · 2020-03-19T19:24:34Z

app/app.go

+	EvidenceKeeper   evidence.Keeper
+	DeploymentKeeper deployment.Keeper
+	MarketKeeper     market.Keeper
+	ProviderKeeper   provider.Keeper


Why are these public?

making the keepers public leverages the AkashApp as the source for integration tests (as it should be). See cosmos/cosmos-sdk#4875 for reference

fwiw I think that the reality is that they don't need to be fields in the struct at all

You need to access all of the keepers?

anyways, please keep them in a separate struct for organization/isolation and only export the keepers that you need to.

fwiw I think that the reality is that they don't need to be fields in the struct at all?

As you prefer, I'd still need to make the keeper internal struct public tho.

You need to access all of the keepers?

I need access to all the keepers used by the Akash modules

ok please keep them in an isolated struct and export them on an as-needed basis.

boz · 2020-03-19T19:30:39Z

app/app.go

+}
+
+// GetMaccPerms returns a copy of the module account permissions
+func GetMaccPerms() map[string][]string {


Why not use the previous definition?

boz · 2020-03-19T19:31:12Z

app/app.go

+	EvidenceKeeper   evidence.Keeper
+	DeploymentKeeper deployment.Keeper
+	MarketKeeper     market.Keeper
+	ProviderKeeper   provider.Keeper


fwiw I think that the reality is that they don't need to be fields in the struct at all

boz · 2020-03-19T19:31:49Z

app/app.go

-func NewApp(
-	logger log.Logger, db dbm.DB, tio io.Writer, options ...func(*bam.BaseApp),
+// NewAkashApp creates and returns a new Akash App.
+func NewAkashApp(


Why are you changing the name?

because the constructor name should match the returned value (i.e AkashApp)

okay. would have preferred to change the struct name to App.

boz · 2020-03-19T19:32:29Z

app/app.go

+		auth.ModuleName, distr.ModuleName, staking.ModuleName, bank.ModuleName,
+		slashing.ModuleName, gov.ModuleName, mint.ModuleName, supply.ModuleName,
+		crisis.ModuleName, genutil.ModuleName, evidence.ModuleName,
+		deployment.ModuleName, provider.ModuleName, market.ModuleName, // akash


why are you changing the format here?

uses too many lines. I usually break the line at ~80 chars

Please keep the previous format.

boz · 2020-03-19T19:33:49Z

app/app.go

-func (app *AkashApp) beginBlocker(
-	ctx sdk.Context, req abci.RequestBeginBlock) abci.ResponseBeginBlock {
+// BeginBlocker application updates every begin block
+func (app *AkashApp) BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock) abci.ResponseBeginBlock {


why are you making these public?

One reason is, it helps in simapp tests

that's the standard of the SDK framework. It doesn´t make much difference since the function calls a private function. Otherwise, you can call the module manager, mm, directly so there's no need to have a private function call another private function imho.

Exporting symbols comes with many costs. Please don't export any symbols unless absolutely necessary.

gosuri · 2020-03-19T20:00:13Z

great! @fedekunze please also to make sure to document your findings and reasons, especially for major interface changes (names, etc) since we have other members in the team that need to rebase and we want to make sure practices are documented well to reduce knowledge asymmetry.

add a section "refactor considerations" for [DCS-5].(https://github.com/ovrclk/dcs/blob/master/spec/dcs-5/README.md)

anilcse · 2020-03-19T20:22:49Z

great! @fedekunze please also to make sure to document your findings and reasons, especially for major interface changes (names, etc) since we have other members in the team that need to rebase and we want to make sure practices are documented well to reduce knowledge asymmetry.

add a section "refactor considerations" for [DCS-5].(https://github.com/ovrclk/dcs/blob/master/spec/dcs-5/README.md)

I checked all these updates and are in sync with the standards sdk/gaia follows. There are a plenty of discussions before making all these changes in gaia and simapp.

I also feel that, keeping akash code close to gaia will help in reducing redundant work for upgrades (sdk changes). Otherwise, it will be an overhead to upgrade.

boz · 2020-03-19T20:52:39Z

The Gia and cosmos-sdk "standards" are constantly-moving targets. They also tend to diverge from idiomatic go patterns.

Refactoring for the sake of refactoring can be necessary at times, but it is not here.

boz · 2020-03-19T20:54:24Z

app/app.go


 		// akash
 		deployment.AppModuleBasic{},
 		market.AppModuleBasic{},
 		provider.AppModuleBasic{},
 	)
+
+	// module account permissions
+	maccPerms = map[string][]string{


the reason for having these be functions was to prevent mutation - the functions return a new copy every time. please revert to the previous pattern.

that's what GetMaccPerms does too 🙂

why did you change it then? before there was no way that it could be unintentionally mutated, and now there is.

Signed-off-by: Federico Kunze <[email protected]>

anilcse · 2020-03-24T07:25:40Z

x/deployment/handler/handler.go

@@ -12,19 +12,19 @@ import (
 func NewHandler(keeper keeper.Keeper, mkeeper MarketKeeper) sdk.Handler {
 	return func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) {
 		switch msg := msg.(type) {
-		case types.MsgCreate:
+		case types.MsgCreateDeployment:


boz

Looking great, thank you!

boz · 2020-03-25T18:22:13Z

x/deployment/types/msgs.go

+const (
+	TypeMsgCreateDeployment = "create_deployment"
+	TypeMsgUpdateDeployment = "update_deployment"
+	TypeMsgCloseDeployment  = "close_deployment"


Can you make these values be the same as those in the codec registration and use these constants there?

If they don't need to be exported, please make them private.

boz · 2020-03-25T18:25:16Z

x/market/types/id.go

+	if id.Owner.Empty() {
+		return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "owner cannot be empty")
+	}
+	if id.DSeq == 0 && id.GSeq == 0 && id.OSeq == 0 {


I believe you can use return id.GroupID().Validate(). Same goes for the rest of them, I'll illustrate below.

I can't bc OSeq is not defined for GroupID

BidID = OrderID + Provider; Validate(BidID) = Validate(OrderID) + Validate(Provider)

OrderID = GroupID + OSeq; Validate(OrderID) = Validate(GroupID) + Validate(OSeq)

GroupID = DeploymentID + GSeq; Validate(GroupID) = Validate(DeploymentID) + Validate(GSeq)

In other words, re-use the validations from the contained structures. Unrolling at every level will lead to bugs, missed validations, and make refactoring more difficult. There are utilities for converting between the various IDs.

See the Equals() methods for an example.

I respectfully disagree, not having any validation or the lack of testing leads to bugs. The same applies for private functions and vars, they don't provide any extra security guarantee if you define the public functions within the scope of the module.

not having any validation or the lack of testing leads to bugs

That's why we have a task to do those things.

Those things are, of course, completely separate from the topic of the benefits of abstraction, unless you count the benefits of re-using already-tested code (when OrderID.Validate() is tested, BidID.Validate() would use that tested code).

The same applies for private functions and vars, they don't provide any extra security guarantee if you define the public functions within the scope of the module.

I don't really know what you mean by that last part, but keeping the surface area of a package to a minimum interface is for abstraction (internal implementation can change without affecting users of the package), isolation (prevents and/or minimizes paths to state mutation), and yes, security (vars can be re-assigned). This is software engineering 101 stuff.

x/market/types/id.go

x/market/types/msgs.go

x/provider/types/msgs.go

boz · 2020-03-25T18:30:00Z

app/app.go

+)
+
+// MaccPerms returns the module account permissions
+func MaccPerms() map[string][]string {


Please just revert this to the original. Does it need to be exported?

boz · 2020-03-25T18:34:24Z

app/app.go

+	// ModuleBasics defines the module BasicManager is in charge of setting up basic,
+	// non-dependant module elements, such as codec registration
+	// and genesis verification.
+	ModuleBasics = module.NewBasicManager(


Please revert this to the original. Exporting it as a function prevents other modules from re-assigning it.

Exporting it as a function prevents other modules from re-assigning it.

modules don't have access to other modules, nor the module manager or other application-level fields unless explicitly given as arguments to the keepers.

sorry, I meant packages as in golang packages.

The fact of the matter is that there was absolutely no reason to change this. The same is true for most of this PR. I won't approve any of these (not necessarily) stylistic changes.

boz · 2020-03-25T18:35:07Z

app/app.go

+		bam.MainStoreKey, auth.StoreKey, staking.StoreKey,
+		supply.StoreKey, mint.StoreKey, distr.StoreKey, slashing.StoreKey,
+		gov.StoreKey, params.StoreKey, evidence.StoreKey,
+		deployment.StoreKey, market.StoreKey, provider.StoreKey,


revert to original format for all of these, please.

boz · 2020-03-25T18:41:22Z

app/app.go

+
+// ModuleAccountAddrs returns all the app's module account addresses.
+func (app *App) ModuleAccountAddrs() map[string]bool {
+	modAccAddrs := make(map[string]bool)


general template like was done originally

coll := generator() vals := make(map[string]bool,len(coll)) for k, v := range coll { // ... } return vals

boz · 2020-03-25T18:43:36Z

x/deployment/types/id.go

+	if id.Owner.Empty() {
+		return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "owner cannot be empty")
+	}
+	if id.DSeq == 0 && id.GSeq == 0 {


return id.DeploymentID().Validate()

ditto, I can't because GSeq is not defined for DeploymentID

GroupID = DeploymentID + GSeq; Validate(GroupID) = Validate(DeploymentID) + Validate(GSeq)

boz · 2020-03-26T02:42:22Z

x/deployment/types/msgs.go

-	TypeMsgCloseDeployment  = "close_deployment"
+	typeMsgCreateDeployment = "create_deployment"
+	typeMsgUpdateDeployment = "update_deployment"
+	typeMsgCloseDeployment  = "close_deployment"


Either

// msgs.go typeMsgCreateDeployment = ModuleName + "/msg-create-deployment" // codec.go cdc.RegisterConcrete(MsgCreateDeployment{}, typeMsgCreateDeployment, nil)

or

// msgs.go typeMsgCreateDeployment = msg-create-deployment" // codec.go cdc.RegisterConcrete(MsgCreateDeployment{}, ModuleName + "/" + typeMsgCreateDeployment, nil)

Same goes for the rest of these.

if you check the godoc for the RegisterConcrete the name must match the type name

great then use

// msgs.go typeMsgCreateDeployment = `MsgCreateDeployment` // codec.go cdc.RegisterConcrete(MsgCreateDeployment{}, ModuleName + "/" + typeMsgCreateDeployment, nil)

I feel like the request was pretty obvious, I don't know why we're still talking about it.

Events have different formats (they use snake case) than amino registration, hence the difference.

I feel like the request was pretty obvious, I don't know why we're still talking about it.

I totally agree, idk why are you still nitpicking on these small things...

I totally agree, idk why are you still nitpicking on these small things...

because you resolved the conversation without saying why you didn't address it.

Can you explain why you think these two things need to have different formats?

The documentation for Type() says:

Returns a human-readable string for the message, intended for utilization within tags

It doesn't say anything about snake case.

Also, it doesn't look like RegisterConcrete() needs to be camel case. In fact, this was working fine before without it. Indeed, the go-amino examples have "anything can go in here if its unique"
in the main README.md:

amino.RegisterConcrete(&MyStruct3{}, "anythingcangoinhereifitsunique", nil)

In the future, please provide sources for your arguments. If you say that the documentation says it must be some way or another, provide a link to that documentation. In this case, it'd be the go-amio godoc for RegisterConcrete() and that Type() should return snake case. Nobody should need to hunt these sources down for you.

idk why are you still nitpicking on these small things...

This was an incredibly innocuous request - use one constant in two places so that the message type name is standardized.

boz · 2020-03-26T02:43:34Z

x/market/types/codec.go

-	cdc.RegisterConcrete(MsgCloseBid{}, ModuleName+"/msg-close-bid", nil)
+	cdc.RegisterConcrete(MsgCloseOrder{}, ModuleName+"/MsgCloseOrder", nil)
+	cdc.RegisterConcrete(MsgCreateBid{}, ModuleName+"/MsgCreateBid", nil)
+	cdc.RegisterConcrete(MsgCloseBid{}, ModuleName+"/MsgCloseBid", nil)


use the constants defined in msgs.go.

use the original names.

boz · 2020-03-26T02:47:47Z

x/market/types/types.go

-	OrderMatched OrderState = iota
-	// OrderClosed is used when state of order is closed
-	OrderClosed OrderState = iota
+	OrderMatched


Closed is not the initial state.

Some linters will complain about this formatting. Why are you changing it to begin with?

the default value for uint8 is 0, that means that all default states would have been open

check https://github.com/uber-go/guide/blob/master/style.md#start-enums-at-one for reference

Some linters will complain about this formatting.

which linters complain about this format?

Yes of course I know the initial value for iota.

When orders are created they are open, hence that being the default state. closed is an end state.

I think it would be reasonable to not use 0 as a value at all to make sure things are always explicit. To do this, replace iota with iota + 1.

I don't remember what linter doesn't like it.

boz · 2020-03-26T02:49:12Z

x/market/types/types.go

-	BidLost BidState = iota
-	// BidClosed is used when state of bid is closed
-	BidClosed BidState = iota
+	BidLost


Same as above.

boz · 2020-03-26T02:49:23Z

x/market/types/types.go

+	// LeaseActive is used when state of lease is active
+	LeaseActive
+	// LeaseInsufficientFunds is used when lease has insufficient funds
+	LeaseInsufficientFunds


boz · 2020-03-26T02:57:52Z

app/test_helpers.go

+)
+
+// Setup initializes a new SimApp. A Nop logger is set in SimApp.
+func Setup(isCheckTx bool) *App {


Is this used for anything other than tests? Does it need access to private fields or functions?

Not sure it should be here. If nothing else, call it SetupTest() and put it in a file that ends in _test.go so that it's only available during testing.

boz · 2020-03-26T16:43:35Z

I think it'd be best to re-start this PR - create a fresh branch and include only the changes necessary for one isolated change - "support using app.App in simulation tests", for instance.

app: refactor

b7b2d14

boz requested changes Mar 19, 2020

View reviewed changes

boz reviewed Mar 19, 2020

View reviewed changes

Merge branch 'master' into fedekunze/457-codebase-migration-refactor

fc49dbd

boz reviewed Mar 19, 2020

View reviewed changes

fedekunze added 3 commits March 23, 2020 09:44

app: address comments from review

161b468

Signed-off-by: Federico Kunze <[email protected]>

x: msg validation

67fc4fc

merge master changes

1f149e5

anilcse reviewed Mar 24, 2020

View reviewed changes

boz requested changes Mar 25, 2020

View reviewed changes

fedekunze added 3 commits March 25, 2020 19:18

x: leverage App for integration and unit testing

b58886b

x: make types private and registered types name must match type name

701ef51

provider: register missign msgs

8d6a792

boz requested changes Mar 26, 2020

View reviewed changes

fedekunze closed this Mar 30, 2020

fedekunze mentioned this pull request Mar 31, 2020

x: rename messages #472

Closed

12 tasks

boz deleted the fedekunze/457-codebase-migration-refactor branch June 10, 2020 19:51

Codebase migration refactor #464

Codebase migration refactor #464

Conversation

fedekunze commented Mar 19, 2020

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

boz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gosuri commented Mar 19, 2020

anilcse commented Mar 19, 2020

boz commented Mar 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boz Mar 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boz Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boz Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

boz Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boz commented Mar 26, 2020

boz Mar 20, 2020 •

edited

Loading

boz Mar 26, 2020 •

edited

Loading

boz Mar 26, 2020 •

edited

Loading

fedekunze Mar 26, 2020 •

edited

Loading

boz Mar 26, 2020 •

edited

Loading