Creating stats/ page with reporting showing where all money goes within a closed system. Drafting DDD. #299

pwdel · 2024-09-07T19:02:28Z

See tickets.
Improvements to testing documentation
Created error helper
Created way to report stats/ to endpoint at api/v0/stats which reports financial stats for a given instance of SocialPredict.
The idea here is that we should be able to calculate and match some given numbers from our state as well, e.g. from wallet ballances. This shows users of the system that nothing, "funny," is going on and that all money created is being accounted for and transparent.
If any market makers are added, transactions should show amounts that have been added to those market makers.
Likewise for bailouts to individual user accounts, debt limits, etc. Everything should make sense and add up to everyone's aggregate wallet balances. This can be built in the future.
Apart from this, created three repositories for data, one for each model, so that we can create an interface between the model and our logic and control how queries happen. Part of my worry was that if we continue writing queries on the fly without a standard logic, we are more likely to create non-performant systems going forward.
Likewise, using a repository system could allow us to work with different types of ORM's or databases. If we end up using a repository and refactor this repository usage into our code, it is going to be much easier to switch out how we interact with a database in the future, if that becomes a need.
I didn't touch the rest of our repo to keep this MR more controlled, but I did do a test to extract PublicUserInfo with our repository model as a demo. Future refactoring using the repository model could be done on a case by case basis for smaller tickets.

Results of my test on the stats/ api

{"moneyIssued":{"totalMoneyIssued":1000,"debtExtendedToRegularUsers":1000,"additionalDebtExtended":0,"debtExtendedToMarketMakers":0},"revenue":{"totalRevenue":139,"transactionFees":9,"marketCreationFees":130},"expenditures":{"totalExpenditures":0,"bonusesPaid":0,"otherExpenditures":0},"fiscalBalance":139,"totalEquity":139,"liabilities":0}

These amounts are expected. I didn't write a test for this yet.

…ate efficient query.

j4qfrost

Left some comments. It looks fine aside from those.

j4qfrost · 2024-09-19T22:52:33Z

backend/handlers/markets/listmarkets.go

@@ -52,7 +52,11 @@ func ListMarketsHandler(w http.ResponseWriter, r *http.Request) {
 		marketVolume := marketmath.GetMarketVolume(bets)
 		lastProbability := probabilityChanges[len(probabilityChanges)-1].Probability

-		creatorInfo := usersHandlers.GetPublicUserInfo(db, market.CreatorUsername)
+		creatorInfo, err := usersHandlers.GetPublicUserInfo(db, market.CreatorUsername)


Should this actually return a 400 or is it really a 404? Digging into the GetPublicUserInfo function it appears both are possible. For now we can handle this as is, but there should be a TODO or something to bubble up the error and handle it accordingly. Admittedly I don't know what errors GetUserByUsername does return.

Also, the market must have a creator username associated with it, so if an error is received, what should or process be for handling it? I don't think failing is the right answer here, but I'm not sure what an effective retry policy would be.

I think this might be a common issue if the creator decides to delete their account. We could create a null user to handle this case or just prevent account deletion unless the user releases the market manually whether by deleting the market or changing "ownership"

404 Error Handling: You are correct that it should be a 404 error when a user is not found, and I will update the logic to reflect that.

Immutable Usernames for Traceability: Usernames should be immutable to maintain traceability of market activities. Allowing users to delete their accounts would remove critical records of the markets they created and how they resolved. Instead, we can implement an archiving system that retains the username but removes sensitive personal information like the display name. This would help prevent users from creating unreliable markets and then disappearing without accountability, ensuring the integrity of the platform.

Put together a decision ticket about this here: https://github.com/orgs/openpredictionmarkets/projects/9/views/1?pane=issue&itemId=80893304

backend/repository/database.go

j4qfrost · 2024-09-19T23:15:50Z

backend/repository/database.go

+	Joins(query string, args ...interface{}) Database
+	Scan(dest interface{}) Database
+	SubQuery() *gorm.DB
+	Raw(sql string, values ...interface{}) Database


We should probably remove this if the intention is to limit functionality.

Interfaces should generally be small, this could be addressed by having something like:

type DatabaseModeler interface { Model(value interface{}) Database }

This would then be used anywhere a Model function is needed. This article provides some further reading on why and how this should be done: https://medium.com/@meeusdylan/go-interfaces-keep-them-small-f148ae200d6b

After looking back over this, I'm not sure about how we should be approaching this in general. What benefits do we get from this organization? i.e. what does a Database interface give us overall?

@ajlacey if the intention is to restrict API access to the ORM I understand the intention of this object, but if we just allow public access to the Raw function, then you can run any command on the DB.

ajlacey

A lot of feedback here, but I think we should go in this order:

Do we need the repository package/database abstraction? If so, what benefits does it give us?
Should the repository package be a separate package or just built on top of the models that are stored in our database type? I think they should be together.
We should look at updating the User type to be composed of its public and private types for convenience.
We should avoid introducing placeholder code. It doesn't give us any functionality and just provides 'dead' code.

Retrospective: I think if we started at 1, we'd be able to decide what that should look like and have an isolated PR just focused on those changes. I think there's also a lot going on here that doesn't appear to be related to creating the stats page (indicative of scope creep). A lot of the new stats code doesn't use the setup config dynamically, which makes it impossible to test the stats code for different scenarios that could arise in the system based on configurations.

ajlacey · 2024-09-21T00:03:51Z

README/BACKEND/GOLANG_CONVENTIONS.md

+## 3. Data Conventions
+
+* The entire application should be as stateless as possible, meaning we have a one-way writeable database of users, markets, bets and calculate all relevant states of the application from as few possible columns within those models.
+* We should separate the data logic from the business logic as much as possible with a Domain Driven Design (DDD), meaning we have a repository/ directory which is designed to be the central location to keep functions that extract data from the databases. This should ideally help slow the growth of the codebase over time and keep data extraction more testable, which should make our startless architecture more reliable.


typo stateless/startless:

which should make our startless architecture more reliable.

ajlacey · 2024-09-21T00:06:55Z

backend/handlers/helpers/error.go

+	if errors.Is(err, gorm.ErrRecordNotFound) {
+		http.Error(w, "User not found", http.StatusNotFound)
+	} else {


what if a different record type isn't found? Why do we only use message in one of the cases here?
Do we need a separate error handler?

Yea I seconded this in my review.

ajlacey · 2024-09-21T00:10:57Z

backend/handlers/users/publicuser.go

+	if err != nil {
+		// Check if the error is because the user was not found
+		if errors.Is(err, gorm.ErrRecordNotFound) {
+			// User not found, return 404 Not Found
+			http.Error(w, "User not found", http.StatusNotFound)
+		} else {
+			// Internal server error
+			http.Error(w, "Internal server error", http.StatusInternalServerError)
+			// Optionally, log the error for debugging purposes
+			log.Printf("Failed to get user info: %v", err)
+		}
+		return
+	}


Note to return to: This feels very similar to the error handler, and I suspect is indicative that the handler isn't necessary.

ajlacey · 2024-09-21T00:12:55Z

backend/handlers/users/publicuser.go

-	db.Where("username = ?", username).First(&user)
+	user, err := repo.GetUserByUsername(username)
+	if err != nil {
+		return PublicUserType{}, fmt.Errorf("failed to get user by username: %w", err)


Instead of returning an empty struct, which will initialize the default zero values, we should return nil, err.
This prevents us from using values that are not initialized in the event that someone calling this function doesn't check the error.

This change will require the return type to be updated to *PublicUserType

ajlacey · 2024-09-21T00:15:48Z

backend/repository/bet_repository.go

These file names are redundant <type>_repository.go should just be <type>.go

ajlacey · 2024-09-21T02:48:01Z

backend/handlers/users/privateuser.go

+	publicInfo, err := GetPublicUserInfo(db, username)
+	if err != nil {
+		helpers.HandleError(w, err, "Error fetching public user info")
+		return
+	}
+


Why does this introduce a new helper to use when the implementation can be done more clearly here?

ajlacey · 2024-09-21T02:50:41Z

backend/handlers/helpers/error.go

I suspect we can remove this ~~file~~ package, but if not we should rename this package: https://go.dev/blog/package-names#bad-package-names

ajlacey · 2024-09-21T02:54:47Z

backend/handlers/markets/listmarkets.go

+			http.Error(w, "Can't retrieve user public info.", http.StatusBadRequest)
+			return


Given the above, if we do return an error here, it should be an internal server error, the request wasn't malformed, the server just failed to retrieve user info.

ajlacey · 2024-09-21T02:54:49Z

backend/handlers/markets/listmarkets.go

@@ -52,7 +52,11 @@ func ListMarketsHandler(w http.ResponseWriter, r *http.Request) {
 		marketVolume := marketmath.GetMarketVolume(bets)
 		lastProbability := probabilityChanges[len(probabilityChanges)-1].Probability

-		creatorInfo := usersHandlers.GetPublicUserInfo(db, market.CreatorUsername)
+		creatorInfo, err := usersHandlers.GetPublicUserInfo(db, market.CreatorUsername)


Also, the market must have a creator username associated with it, so if an error is received, what should or process be for handling it? I don't think failing is the right answer here, but I'm not sure what an effective retry policy would be.

ajlacey · 2024-09-21T03:00:27Z

backend/handlers/stats/revenue.go

A lot of these functions have a dependency on the setup config, but don't inject that dependency into their functionality. This needs to be fixed to test these helpers properly.

j4qfrost · 2024-09-21T04:35:15Z

Thinking more on this, the PR should be broken down, not just because it touches a lot of files, there are multiple objectives.

pwdel · 2024-09-24T15:32:37Z

A lot of feedback here, but I think we should go in this order:
1. Do we need the repository package/database abstraction? If so, what benefits does it give us?

2. Should the `repository` package be a separate package or just built on top of the models that are stored in our database type? I think they should be together.

3. We should look at updating the `User` type to be composed of its public and private types for convenience.

4. We should avoid introducing placeholder code. It doesn't give us any functionality and just provides 'dead' code.
Retrospective: I think if we started at 1, we'd be able to decide what that should look like and have an isolated PR just focused on those changes. I think there's also a lot going on here that doesn't appear to be related to creating the stats page (indicative of scope creep). A lot of the new stats code doesn't use the setup config dynamically, which makes it impossible to test the stats code for different scenarios that could arise in the system based on configurations.

Put together a ticket on point 1. Other points are separate because I'll remove the extra, non-used functions and we could take care of Public/Private user model in a separate ticket. In fact I'll put another ticket together for that right now as well.

Add correction for total money in system per usercount.

79ec535

pwdel self-assigned this Sep 7, 2024

pwdel marked this pull request as draft September 7, 2024 19:02

pwdel linked an issue Sep 7, 2024 that may be closed by this pull request

Stats Page with Accounting for All Money Created and Used In System, -> Towards Being Able To Calculate Against All Wallets #192

Open

pwdel added 3 commits September 8, 2024 14:48

Updating financial stats to be more comprehensible.

05bbfc7

Drafting out fees calculation.

dfd8184

Updating, attempting to add in repository architecture.

7bb7129

pwdel changed the title ~~Add correction for total money in system per usercount.~~ Creating stats/ page with reporting showing where all money goes within a closed system. Drafting DDD. Sep 9, 2024

pwdel linked an issue Sep 9, 2024 that may be closed by this pull request

Create Data Repository Layer for More Precise Testing + Documentation #311

Open

pwdel added 13 commits September 9, 2024 10:47

Adding updated stats to calculate fees.

f62b2c9

Adding conventions.

78037c7

Merge branch 'main' into reportingstats

d10a142

Updating tested repo functions.

ce82662

Adding with all tests working.

ff13145

Updating, attempting to draft firsttimebet count repo function to cre…

07b74de

…ate efficient query.

Tests passing after new methods added.

2e749da

Saving work, sql syntax error.

6a66ec7

Hopefully fixing FirstTimeBets() repo function.

b3b90a5

Fix getuserbyusername.

c2c610f

Transaction fees from initial transactions working.

cfda87e

Adding count regular users, various fixes feeding into stats page.

7505f72

Updating to reflect expenditure calculation justification.

dccadd0

pwdel marked this pull request as ready for review September 19, 2024 18:18

pwdel requested review from ajlacey and j4qfrost September 19, 2024 18:18

j4qfrost requested changes Sep 20, 2024

View reviewed changes

ajlacey requested changes Sep 21, 2024

View reviewed changes

Merge branch 'main' into reportingstats

74eba1c

pwdel mentioned this pull request Sep 24, 2024

Immutable Account Names, Account Traceability #342

Open

pwdel added 2 commits September 24, 2024 07:43

Merge branch 'main' into reportingstats

ae634d0

Fixing typo.

7b491fa

pwdel mentioned this pull request Sep 24, 2024

Data Repository #343

Open

pwdel mentioned this pull request Sep 24, 2024

Separate Public/Private User Types for Convenience #344

Open

Saving last changes, don't remember what they were.

da7eb4b

pwdel mentioned this pull request Sep 27, 2024

Moving golang conventions documentation to own branch. #345

Merged

pwdel marked this pull request as draft October 2, 2024 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating stats/ page with reporting showing where all money goes within a closed system. Drafting DDD. #299

Creating stats/ page with reporting showing where all money goes within a closed system. Drafting DDD. #299

pwdel commented Sep 7, 2024 •

edited

Loading

j4qfrost left a comment

j4qfrost Sep 19, 2024

ajlacey Sep 21, 2024

j4qfrost Sep 21, 2024

pwdel Sep 24, 2024

j4qfrost Sep 19, 2024

ajlacey Sep 21, 2024

ajlacey Sep 21, 2024

j4qfrost Sep 21, 2024

ajlacey left a comment

ajlacey Sep 21, 2024

ajlacey Sep 21, 2024

j4qfrost Sep 21, 2024

ajlacey Sep 21, 2024

ajlacey Sep 21, 2024

ajlacey Sep 21, 2024

ajlacey Sep 21, 2024

ajlacey Sep 21, 2024

ajlacey Sep 21, 2024

ajlacey Sep 21, 2024

ajlacey Sep 21, 2024

j4qfrost commented Sep 21, 2024

pwdel commented Sep 24, 2024

		http.Error(w, "Can't retrieve user public info.", http.StatusBadRequest)
		return

Creating stats/ page with reporting showing where all money goes within a closed system. Drafting DDD. #299

Are you sure you want to change the base?

Creating stats/ page with reporting showing where all money goes within a closed system. Drafting DDD. #299

Conversation

pwdel commented Sep 7, 2024 • edited Loading

j4qfrost 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

ajlacey 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

j4qfrost commented Sep 21, 2024

pwdel commented Sep 24, 2024

pwdel commented Sep 7, 2024 •

edited

Loading