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

[TECHDEBT] Consolidated PostgresContext and PostgresDB (#149) #219

Merged
merged 14 commits into from
Sep 23, 2022

Conversation

Olshansk
Copy link
Member

@Olshansk Olshansk commented Sep 14, 2022

Description

Tend to tech debt in the persistence module related to simplifying the PostgresContext struct.

Issue

Fixes #149

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix (non-breaking change which fixes an issue)
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other: <REPLACE_ME>

List of changes

  • Consolidates PostgresContext and PostgresDB
  • Reduces scope of Tx and BlockStore
  • Improve error handling in NewFuzzTestPostgresContext

Testing

  • make test_all
  • LocalNet w/ all of the steps outlined in the README

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my changes using the available tooling
  • If applicable, I have made corresponding changes to related local or global README
  • If applicable, I have added new diagrams using mermaid.js
  • If applicable, I have added tests that prove my fix is effective or that my feature works

@Olshansk Olshansk added persistence Persistence specific changes code health Nice to have code improvement labels Sep 14, 2022
@Olshansk Olshansk self-assigned this Sep 14, 2022
@Olshansk Olshansk requested a review from a team as a code owner September 14, 2022 19:53
shared/node.go Outdated Show resolved Hide resolved
persistence/test/setup_test.go Outdated Show resolved Hide resolved
persistence/CHANGELOG.md Outdated Show resolved Hide resolved
persistence/context.go Outdated Show resolved Hide resolved
Copy link
Contributor

@andrewnguyen22 andrewnguyen22 left a comment

Choose a reason for hiding this comment

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

You didn't complete the original ask about consolidating txn and tx.

Let me add more detail:

type PostgresContext struct {
	Height     int64 // TODO(olshansky): `Height` is only externalized for testing purposes. Replace with helpers...
	conn       *pgx.Conn
	tx         pgx.Tx                          // This field is `tx`
	blockstore kvstore.KVStore
}
// These functions use Txn() 

func (pg *PostgresContext) GetCtxAndTxn() (context.Context, pgx.Tx, error) {
	return context.TODO(), pg.GetTxn(), nil
}

func (pg *PostgresContext) GetTxn() pgx.Tx {
	return pg.tx
}

Let's consolidate this terminology in order to lower cognitive load and increase readability

Also, please ensure that the structure field uses the GetTxn() function instead of accessing the private field directly:

Ex.

if err := p.tx.Rollback(ctx); err != nil {
		return err
	}

@Olshansk
Copy link
Member Author

Got it now, ty! I def like this better.

Also, pro tip for markdown. You can add syntax highlighting by specifying the language after the triple backtick:
Screen Shot 2022-09-19 at 3 22 09 PM
Screen Shot 2022-09-19 at 3 30 28 PM
Screen Shot 2022-09-19 at 3 22 18 PM

Copy link
Contributor

@andrewnguyen22 andrewnguyen22 left a comment

Choose a reason for hiding this comment

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

Please change this PR description to fill out the template provided

I don't believe it's as clear to the outsider.

Also, consider adding more detail to the changelog for tracking purposes

if strings.Contains(newActor.StakedTokens, "invalid") {
fmt.Println("")
}
// if strings.Contains(newActor.StakedTokens, "invalid") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this commented out?

Also, if you're going to touch this code, it seems like you could easily satisfy that TODO there.

Perhaps it's out of scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was mainly to tend to require.NotContains(t, newActor.StakedTokens, "invalid") given the require.NotContains requirment above.

You're correct that it's out-of-scope so I reverted the changes and updated the comment

@@ -23,7 +23,7 @@ func (p PostgresContext) GetAccountAmount(address []byte, height int64) (amount
}

func (p PostgresContext) getAccountAmountStr(address string, height int64) (amount string, err error) {
ctx, txn, err := p.DB.GetCtxAndTxn()
ctx, txn, err := p.GetCtxAndTx()
Copy link
Contributor

@andrewnguyen22 andrewnguyen22 Sep 20, 2022

Choose a reason for hiding this comment

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

I don't want to see txn anywhere in this code anymore. Let's keep it consistent

Examples:

ctx, txn, err := p.GetCtxAndTx()
func (p *PostgresContext) GetChainsForActor(
	ctx context.Context,
	txn pgx.Tx,

Copy link
Member Author

Choose a reason for hiding this comment

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

I like it! Done

@Olshansk Olshansk changed the title [TECHDEBT] Consolidated PostgresContext and PostgresDB [TECHDEBT] Consolidated PostgresContext and PostgresDB (#149) Sep 20, 2022
Copy link
Contributor

@andrewnguyen22 andrewnguyen22 left a comment

Choose a reason for hiding this comment

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

Resolve conflicts then approve incoming!

@Olshansk
Copy link
Member Author

@andrewnguyen22 Merged with main and pushed.

@Olshansk Olshansk merged commit 82df934 into main Sep 23, 2022
@Olshansk Olshansk deleted the issue_149/consolidate_postgres branch September 23, 2022 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Nice to have code improvement persistence Persistence specific changes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Persistence] Fix Persistence Module TODOs - Consolidate Postgres
2 participants