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

[Configuration] Genesis and Node Configurations (Issue#154) #166

Merged
merged 27 commits into from
Aug 24, 2022
Merged

Conversation

andrewnguyen22
Copy link
Contributor

Intention

Currently merge ready with #128 but not considering #165 since it's under active development.

Description

  • Deprecated old placeholder genesis_state and genesis_config
  • Added utility_genesis_state to genesis_state
  • Added consensus_genesis_state to genesis_state
  • Added genesis_time to consensus_genesis_state
  • Added chainID to consensus_genesis_state
  • Added max_block_bytes to consensus_genesis_state
  • Added accounts and pools to utility_genesis_state
  • Added validators to utility_genesis_state
  • Added applications to utility_genesis_state
  • Added service_nodes to utility_genesis_state
  • Added fishermen to utility_genesis_state
  • Deprecated shared/config/
  • Added new shared config proto3 structure
  • Added base_config to config
  • Added utility_config to config
  • Added consensus_config to config
  • Added persistence_config to config
  • Added p2p_config to config
  • Added telemetry_config to config
  • Opened followup issue [Core] Minimize shared module #163
  • Added config and genesis generator to build package
  • Deprecated old build files
  • Use new config and genesis files for make compose_and_watch
  • Use new config and genesis files for make client_start && make client_connect

closes #154

Type of change

Please mark the options that are relevant.

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation
  • Other (<REPLACE_ME_WITH_DETAILS>)

How Has This Been Tested?

make test_all
&&
make compose_and_watch

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 or global README
  • If applicable, I have added added new diagrams using mermaid.js
  • If applicable, I have added tests that prove my fix is effective or that my feature works

Andrew Nguyen and others added 16 commits July 6, 2022 16:49
#73)

## Objective

Foundational iteration of PostgreSQL based persistence module implementation.

## Origin Document

#68

## Type of change

New major module implementation.

### Persistence-related core changes:
- List of actors / interfaces with MVP implementation:
    - Applications
    - Fisherman
    - ServiceNode
    - Accounts
    - Pools
- List of actors / interfaces with partial MVP implementation:
    - Validator
    - Gov params
- List of actors / interfaces with minorimplementation:
    - Block
- SQL Schema definition of the actors above
- SQL Query implementation for common actor persistence functionality
- PostgresContext implementation of the actors actors above
- Base infrastructure for fuzz testing

Non-persistence “fly-by” changes
- Updates to the PrePersistence module and utility module with breaking changes
- A few minor improvements/additions to the Makefile
- TODOs & comment cleanups throughout the codebase

## How Has This Been Tested?

### Unit Tests

```
make test_persistence
make test_all
```

### LocalNet
Ran a basic LocalNet following the instructions in the [development README](docs/development/README.md).


Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MacBook-Pro-2.local>
Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MBP-2.lan>
Co-authored-by: Daniel Olshansky <olshansky@pokt.network>
@andrewnguyen22 andrewnguyen22 added core Core infrastructure - protocol related code health Nice to have code improvement labels Aug 14, 2022
@andrewnguyen22 andrewnguyen22 self-assigned this Aug 14, 2022
@andrewnguyen22 andrewnguyen22 requested a review from a team as a code owner August 14, 2022 22:41
@Olshansk Olshansk changed the base branch from main to issue-#128 August 14, 2022 22:51
@Olshansk
Copy link
Member

Personal request: there are a few bugs I've found in #128 (not just code cleanup), so I wanted to prioritize #165 first before we review this one.

Also, make sure that both #128 and #166 are up to sync with master because when we change the base, I'm seeing unrelated diffs:

Screen Shot 2022-08-14 at 6 54 16 PM

@andrewnguyen22
Copy link
Contributor Author

Personal request: there are a few bugs I've found in #128 (not just code cleanup), so I wanted to prioritize #165 first before we review this one.

Also, make sure that both #128 and #166 are up to sync with master because when we change the base, I'm seeing unrelated diffs:

Screen Shot 2022-08-14 at 6 54 16 PM

Yeah sounds good on the ordering.

Like 90% sure you had merged #128 with main and then force pushed it to revert it lol

I merged #128 with main so all should be good now

Base automatically changed from issue-#128 to main August 15, 2022 21:50
@Olshansk Olshansk changed the title Issue #154 [Configuration] Genesis and Node Configurations (Issue#154) Aug 22, 2022
@Olshansk
Copy link
Member

@andrewnguyen22 As discussed offline, I have done the following:

  1. Reviewed this PR
  2. Left most things I would have left as PR comments as TODOs in this PR: [Configuration] PR166 Code Review (Issue#154 Follow ups) #171
  3. Created an issue to track this work here: [TECHDEBT] Persistence code-review follow (tend TODOs added via a PR rather than a code review) #172

Once #171 is approved, we can do the following:

  1. Merge in this PR
  2. Merge in [Configuration] PR166 Code Review (Issue#154 Follow ups) #171
  3. Review [Core] Minimize shared module #163
  4. Determine what changes in [Core] Minimize shared module #163 should be made and what can be left to a follow up
  5. Merge in [Core] Minimize shared module #163
  6. Start tending to the tasks here

Lmk what you think of the plan

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Approving this merge since #171 is coming in immediately after. Will take care of bringing both of them in.

@Olshansk Olshansk merged commit eb9d94a into main Aug 24, 2022
@Olshansk Olshansk deleted the issue-#154 branch August 24, 2022 15:07
Olshansk added a commit that referenced this pull request Aug 24, 2022
## Description
This is a code review of #166.

Rather than leaving comments on the original PR, we are leaving TODOs throughout the code with only a few changes being pushed right now. They will be tended to in this issue: https://github.com/pokt-network/pocket/issues/<ISSUE_NUMBER>

## Type of change
Please mark the options that are relevant.

- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Documentation
- [X] Other: Code cleanup

## How Has This Been Tested?

- [ ] `make test_all`
- [ ] [LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md)

## 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 or global README
- [ ] If applicable, I have added added new diagrams using [mermaid.js](https://mermaid-js.github.io)
- [ ] If applicable, I have added tests that prove my fix is effective or that my feature works
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 core Core infrastructure - protocol related
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Genesis and Config] Replace the placeholders with mvp implementations
2 participants