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.

Sign up for GitHub

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

Jump to bottom

[Service] NIT followups to AddService in #316 #338

Merged
merged 25 commits into from
Jan 19, 2024
Merged

[Service] NIT followups to AddService in #316 #338

merged 25 commits into from
Jan 19, 2024

Conversation

Olshansk
Copy link
Member

Summary

NIT followups to #316 added in a separate PR to avoid blocking the base PR.

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 go_develop_and_test
  • Run E2E tests locally: make test_e2e
  • Run E2E tests on DevNet: Add the devnet-test-e2e label to the PR. This is VERY expensive, only do it after all the reviews are complete.

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

h5law and others added 20 commits January 12, 2024 16:53
feat: add genesis ServiceList and genesis validation and tests
dfe9ef0
feat: add more message validation and initial implementation
1c14818
feat: add CRUD operations for the service keeper
62046e4
feat: add service keeper functions and tests
da1b4d3
feat: implement the add-service CLI command
cc0c8a3
feat: update genesis and simulation files
f607760
feat: implement AddService
8ce4697
feat: renaming, minor refactor in simulation
14fddcb
feat: add info log line when adding service
cf031bf
feat: add CmdAddService tx tests
d0dab74
feat: add message handler tests
193e454
chore: localnet residuals
099f368
chore: rename test struct fields
cb7cc56
Merge branch 'main' into feat/add-service-impl
af951c9
chore: ignore mise local config file
cd73b31
chore: address comments
7cba05f
chore: ignite chain build
f651a49
chore: address comments
031956f
Merge branch 'main' into feat/add-service-impl
ad2b93d
@Olshansk
Ran make go_lint && make go_imports
bcaf2fa
@Olshansk Olshansk added documentation Improvements or additions to documentation on-chain On-chain business logic code health Cleans up some code labels Jan 19, 2024
@Olshansk Olshansk added this to the Shannon TestNet milestone Jan 19, 2024
@Olshansk Olshansk requested a review from h5law January 19, 2024 16:29
@Olshansk Olshansk self-assigned this Jan 19, 2024
@Olshansk Olshansk mentioned this pull request Jan 19, 2024
Merged
11 tasks
h5law
h5law approved these changes Jan 19, 2024
View reviewed changes
Copy link
Contributor

@h5law h5law left a comment

Choose a reason for hiding this comment

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

Left a few nits on imports but gunna approve please fix then merge <3

x/service/client/cli/tx_add_service_test.go Outdated Show resolved Hide resolved
x/service/keeper/service_test.go Outdated
"github.com/pokt-network/poktroll/cmd/pocketd/cmd"
keepertest "github.com/pokt-network/poktroll/testutil/keeper"
"github.com/pokt-network/poktroll/testutil/nullify"
"github.com/pokt-network/poktroll/x/service/keeper"
"github.com/pokt-network/poktroll/x/service/types"
sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
"github.com/stretchr/testify/require"
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Move this back to second block.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have any idea why this doesn't format it correctly?

Screenshot 2024-01-19 at 2 57 54 PM

Copy link
Contributor

@h5law h5law Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/incu6us/goimports-reviser

https://github.com/golang/vscode-go/wiki/features#format-and-organize-imports

https://github.com/golang/vscode-go/wiki/advanced#formatting-code-and-organizing-imports

poktroll/.golangci.yml

Line 30 in 943669e

gci:

These links should help, I don't know exactly as I don't use vscode - but the last code reference to my linting PR should help as a reference on the 2nd link.

Copy link
Contributor

Choose a reason for hiding this comment

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

goimports != goimports-reviser you want a final group which is prefixed with our repo name. Pretty sure there is a goimports-reviser plugin maybe for vscode but whatever way you find to make it work - document it somewhere so we can make a configs section

Copy link
Contributor

@h5law h5law Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

https://chat.openai.com/share/d2590505-31e2-4e34-be93-8e4fabf8d872 @Olshansk this should do it ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

incu6us/goimports-reviser#147 🤷

Base automatically changed from feat/add-service-impl to main January 19, 2024 17:41
@Olshansk Olshansk force-pushed the feat/add-service-impl-nits branch from 3dd2de0 to ea72e97 Compare January 19, 2024 20:01
@github-actions GitHub Actions
Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. If you just created a pull request, you might need to push another commit to produce a container image DevNet can utilize to spin up infrastructure. You can use make trigger_ci to push an empty commit.

@github-actions github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Jan 19, 2024
@Olshansk
Copy link
Member Author

@h5law Fixed the imports and will merge this in after E2E tests pass. Plz see my comment in #338 (comment), since I'm trying to understand what formatted you use.

@Olshansk Olshansk merged commit 9aabe7d into main Jan 19, 2024
11 checks passed
@Olshansk Olshansk deleted the feat/add-service-impl-nits branch January 19, 2024 21:41
bryanchriswhite added a commit that referenced this pull request Jan 22, 2024
@bryanchriswhite
Merge remote-tracking branch 'pokt/main' into issues/141/refactor/pro…
7e8cb19 
…of-store-indices-2

* pokt/main:
  [Off-chain] Simplify `TxClient` with `EventsQueryClient` (#330)
  fix: add yarn.lock (#334)
  [Service] NIT followups to `AddService` in #316 (#338)
  [Service] Implement AddService with extensive tests (#316)
  [Supplier] refactor: proof store indices (2) (#327)
@bryanchriswhite bryanchriswhite removed push-image CI related - pushes images to ghcr.io devnet-test-e2e labels May 16, 2024
@github-actions github-actions bot removed the devnet label May 16, 2024
okdas pushed a commit that referenced this pull request Nov 14, 2024
@Olshansk
[Service] NIT followups to AddService in #316 (#338)
08243a5 
NIT followups to #316 added in a separate PR to avoid blocking the base PR.

---------

Co-authored-by: h5law <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h5law h5law h5law approved these changes

Assignees

@Olshansk Olshansk

Labels
code health Cleans up some code documentation Improvements or additions to documentation on-chain On-chain business logic
Projects
Shannon
Status: ✅ Done
Milestone
Shannon Private TestNet
Development

Successfully merging this pull request may close these issues.
3 participants
@Olshansk @h5law @bryanchriswhite