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

Feature/market creation interface #327

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

ajlacey
Copy link
Collaborator

@ajlacey ajlacey commented Sep 14, 2024

This PR adds an interface type for the MarketCreation component of our setup config. This interface is implemented by both the EconomicConfig and MarketCreation types. There are a number of pieces of code that are only dependent on the MarketCreation component of the config, and this allows us to use any types that can return a MarketCreation in our code and tests.

ajlacey and others added 25 commits September 8, 2024 22:55
removed gorm sqlite driver because it requires cgo to be enabled
replaced with glebarez/sqlite which is a fork of of the gorm sqlite
driver that does not use cgo.
…r version. Update migration errors and comments
Bug report template updated
When running the below commands, a *.out and cover.html file are generated. We may want to make these patterns more generic, but for now this will prevent them from being tracked in the backend.

go test -coverprofile=c.out ./...
go tool cover -html=c.out -o=cover.html
@ajlacey ajlacey requested review from pwdel and j4qfrost September 29, 2024 05:39
@ajlacey ajlacey marked this pull request as ready for review September 29, 2024 05:40
type MarketCreationLoader interface {
LoadMarketCreation() MarketCreation
}

Copy link
Member

Choose a reason for hiding this comment

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

Focusing on this part of the code, I was struggling to understand why, "market creation," is a part of the setup. However after reviewing the entire codebase, I see that it's simply a way of creating brevity, e.g. so we don't have to do: this.thing.then.this.thing ... Part of my confusion I think was, the act of creating a market is an action that you can do within our application, e.g., "create market," whereas, this is more like, a state, with parameters used in the process of creating a market.

Would MarketCreationCostsLoader or, MarketCreationIncentivesLoader be any clearer from your point of view?

I also understand the desire to keep variable names shorter, but at least at work, I feel like there are a lot of super short names which are shared between different abstractions. E.g., "data," is used, rather than, "runInputData" is the convention, however you might see, "data" used in different ways across different parts of our software.

Might be an aesthetic preference, might be a way of making things more clear. What are your thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think if we want to rename this interface we should consider renaming the MarketCreation type as well.
This could be a setup.Market, setup.InitialMarket (and then rename the fields within to be less redundant). We're going to be using the setup consistently throughout this code base, so it might be worth having a video call / brainstorming session to think about the ergonomics of these names.

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.

Create smaller interfaces for returning the relevant portions of the economics config
2 participants