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

[Application] Implement MsgStakeApplication & Add Extensive Tests #59

Merged
merged 55 commits into from
Oct 18, 2023

Conversation

Olshansk
Copy link
Member

@Olshansk Olshansk commented Oct 11, 2023

Summary

Screen.Recording.2023-10-16.at.5.11.57.PM.mov

Human Summary

  • Add stake to the Application type
  • Add stake to the StakeApplication command
  • Implement unit tests in all related parts of the codebase (CLI, genesis, keeper, etc...) with the exception of the simulation package
  • Small helpers (e.g. Makefile)

Auto-Generated Summary

Summary generated by Reviewpad on 18 Oct 23 14:36 UTC

This pull request includes the following changes:

  1. In the file README.md, the project name has been changed from "pocket" to "poktroll". The project description has also been updated to reflect the use of Rollkit, Cosmos SDK, and CometBFT for the Shannon upgrade of the Pocket Network blockchain. The sections "Getting Started" and "Release" have been added, while the sections "Configure", "Web Frontend", and "Learn more" have been removed. The "Install" section has been updated with instructions for installing the latest version of the blockchain node's binary.

  2. A new file tx_stake_application_test.go has been added in the cli package of the x/application module. This file contains test cases for the stake application CLI command. The tests cover various scenarios, such as staking with valid inputs, missing address, invalid address, missing stake, invalid stake denom, and invalid stake amount.

  3. In a file, a new import statement for the package github.com/cometbft/cometbft-db has been added. Additionally, some import statements have been updated and commented. A function initSDKConfig() has been changed to InitSDKConfig().

  4. In the file go.mod, some changes have been made to the "require" section. New modules have been added, and one module has been removed. The google.golang.org/genproto module has been updated.

  5. The file config.yml has undergone changes. The names of some accounts in the accounts section have been modified. The genesis section has been updated with additional information and comments.

  6. Changes have been made to the file application.go. An import statement for github.com/cosmos/cosmos-sdk/types has been added, and the types package from pocket/x/application has been imported.

  7. A new mock for the BankKeeper has been added in a file. The file also includes modifications to the ApplicationKeeper to use this mock.

  8. The file errors.go has been updated with changes to import statements and the addition of new error variables.

  9. The file genesis_test.go has been modified with changes to import statements, comments, and the TestGenesis function.

  10. The file MessageStakeApplication in the types package has been updated with changes to import statements and the addition of a new field.

  11. The .gitignore file has undergone changes, including the addition of patterns to ignore specific file extensions.

  12. Modifications have been made to the Makefile, including changes to variables, the addition of new commands, and reordering of existing commands.

  13. The openapi.yml file has been changed with additions to the application object in multiple places.

  14. The file genesis.go has been updated with changes to import statements and the addition of validation logic.

  15. The file app.go has been modified with changes to import statements, the introduction of a constant, and the addition of a comment.

  16. The file mocks.go has been added, providing documentation on dynamically generated structs.

  17. The file tx_stake_application.go has been modified with changes to import statements, command descriptions, and the addition of logic to parse and validate the stake amount.

  18. The file keeper_test.go has been added, containing test cases for the MsgServer_StakeApplication function in the keeper package.

  19. The file genesis_test.go has been modified with changes to import statements and the addition of test cases for the validation of the GenesisState struct.

  20. The file sample_test.go has been added, containing test cases for validating the ApplicationList field in the GenesisState struct.

  21. The file .github/workflows/go.yml has undergone changes, involving reordering of steps and modifications to the "Test" step.

This pull request includes various updates and additions to the project documentation, test files, import statements, command functionalities, and workflow configurations.

Issue

Part of #8

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make test_all_unit
  • Verify Localnet manually: See the instructions [here](TODO: add link to instructions)

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have performed a self-review of my own code
  • I have commented my code, updated documentation and left TODOs throughout the codebase

@Olshansk Olshansk added the application Changes related to the Application actor label Oct 11, 2023
@Olshansk Olshansk self-assigned this Oct 11, 2023
@Olshansk Olshansk added this to the Shannon TestNet milestone Oct 11, 2023
@Olshansk Olshansk marked this pull request as ready for review October 13, 2023 00:16
@Olshansk Olshansk changed the title [WIP][Application] Application Stake Command & Tests [Application] Application Stake Command & Tests Oct 13, 2023
@@ -6,7 +6,7 @@ import (
"pocket/app"
)

func initSDKConfig() {
func InitSDKConfig() {
Copy link
Contributor

@bryanchriswhite bryanchriswhite Oct 13, 2023

Choose a reason for hiding this comment

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

If this is only being exposed for testing purposes, wdyt about adding a test_config.go file which exposes a wrapper and has a //go:build test tag/constraint?:

//go:build test

package cmd

// InitSDKConfig exports initAppConfig for use in tests
func InitSDKConfig() {
	initSDKConfig()
}

(see: https://github.com/pokt-network/poktroll/pull/42/files#diff-fe3797aa7d51913c0f4b02574c878cf02042fb8a159d5355a2c1b3731490b982R1)

We would have to update the makefile as well:

.PHONY: go_test
go_test: go_version_check ## Run all go tests
	go test -v -tags test ./...

(see: https://github.com/pokt-network/poktroll/pull/42/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R94-R98)

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 gave this a shot, but not a fan that new go developers would be confused by the fact that go test ./... doesn't work out of the box. Added a TODO_DISCUSS for now and will revisit after this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Previously we had discussed adding //go:build test to all test files to mitigate this concern. The user experience would be that without the -tags test constraint, the go test would say that "no tests were run".

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 added a TODO link to this discussion. Is it alright if follow up on it outside of this PR?

If we do go with this approach, we should have really good CLI communication to the user to make it obvious how necessary the tag is like YOU HAVE NOT ADDED THIS TAG SO NO TESTS ARE BEING EXECUTED.

Copy link
Contributor

@bryanchriswhite bryanchriswhite Oct 18, 2023

Choose a reason for hiding this comment

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

For sure, we can continue that discussion after this PR.

I'm just now recalling that had also done what you're suggesting 😂. IIRC, there was a test which did not have the test build constraint and would check whether the -tags flag contained test, and return a similar error if not. I'll see if I can find it but otherwise, it should be straightforward to reproduce.

config.yml Outdated Show resolved Hide resolved
@Olshansk Olshansk changed the title [Application] Application Stake Command & Tests [Application] Implement MsgApplicationStake Transaction & Tests Oct 17, 2023
@Olshansk Olshansk changed the title [Application] Implement MsgApplicationStake Transaction & Tests [Application] Implement MsgApplicationStake & Add Tests Oct 17, 2023
@Olshansk Olshansk changed the title [Application] Implement MsgApplicationStake & Add Tests [Application] Implement MsgStakeApplication & Add Extensive Tests Oct 18, 2023
@Olshansk Olshansk merged commit b5cb88d into main Oct 18, 2023
3 checks passed
@Olshansk Olshansk deleted the issues/8/application_stake branch October 18, 2023 14:59
bryanchriswhite added a commit that referenced this pull request Oct 19, 2023
* main:
  [Miner, Supplier] chore: scaffold submit-proof message (#44)
  [Application] Implement MsgUnstakeApplication & Add Extensive Tests (#72)
  [Gateway] Implement UnstakeGateway message and add Tests (#75)
  [Gateway] Implement StakeGateway Message and Add Tests (#68)
  [Application] Implement MsgStakeApplication & Add Extensive Tests (#59)
  [CI] fix: run `make go_mockgen` step (#63)
bryanchriswhite added a commit that referenced this pull request Oct 19, 2023
* feat/observable:
  chore: last minute improvements
  chore: review improvements
  chore: improve comment
  chore: misc. review feedback improvements
  chore: improve comments
  refactor: rename `Observable#Close()` to `#UnsubscribeAll()`
  chore: improve comments
  chore: improve comments
  chore: cleanup, simplification, review improvements
  [Miner, Supplier] chore: scaffold submit-proof message (#44)
  [Application] Implement MsgUnstakeApplication & Add Extensive Tests (#72)
  [Gateway] Implement UnstakeGateway message and add Tests (#75)
  [Gateway] Implement StakeGateway Message and Add Tests (#68)
  [Application] Implement MsgStakeApplication & Add Extensive Tests (#59)
  [CI] fix: run `make go_mockgen` step (#63)
okdas pushed a commit that referenced this pull request Nov 14, 2024
## Summary

https://github.com/pokt-network/poktroll/assets/1892194/ffe3158d-a1b3-4672-8e0b-e673349067d2

### Human Summary

- Various small helpers (e.g. Makefile)
- Shared helpers for unit tests
- Add `stake` to the `Application` type
- Add `stake` to the `MsgStakeApplication` command
- Implement unit tests in all related parts of the codebase (CLI, genesis, keeper, etc...) with the exception of the `simulation` package

--- 

Co-authored-by: harry <[email protected]>
Co-authored-by: Bryan White <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application Changes related to the Application actor
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants