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

[CLI] Filter Suppliers by ServiceID #1028

Merged
merged 25 commits into from
Jan 20, 2025
Merged

[CLI] Filter Suppliers by ServiceID #1028

merged 25 commits into from
Jan 20, 2025

Conversation

Olshansk
Copy link
Member

@Olshansk Olshansk commented Jan 13, 2025

Summary

Update the supplier query endpoint to use AutoCLI and add a flag to filter by ServiceID

Primary Changes:

  • Add service_id filter to list-suppliers query to support filtering suppliers by service ID
  • Update CLI command from list-supplier to list-suppliers for improved clarity
  • Change revenue share percentage field type from float32 to uint32 to fix autoCLI issues

Secondary changes:

  • Update documentation and config examples to reflect the new revenue share percentage type
  • Remove legacy CLI commands in favor of autocli implementation
Screenshot 2025-01-16 at 12 58 06 PM Screenshot 2025-01-16 at 12 57 52 PM

Type of change

Select one or more from the following:

Testing

  • Documentation: make docusaurus_start; only needed if you make doc changes
  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code
  • I create and reference any new tickets, if applicable
  • I have left TODOs throughout the codebase, if applicable

poktrolld --home=./localnet/poktrolld q supplier list-suppliers --service-id abc
poktrolld --home=./localnet/poktrolld q supplier list-suppliers --service-id ollama
@Olshansk Olshansk self-assigned this Jan 14, 2025
@Olshansk Olshansk added on-chain On-chain business logic tooling Tooling - CLI, scripts, helpers, off-chain, etc... labels Jan 14, 2025
@Olshansk Olshansk added this to the Beta TestNet Iteration milestone Jan 14, 2025
@Olshansk Olshansk changed the title [WIP] AutoCLI + Add flag to filter suppliers [CLI] Filter Suppliers by ServiceID Jan 16, 2025
@Olshansk Olshansk marked this pull request as ready for review January 16, 2025 17:58
Copy link
Contributor

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

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

@Olshansk, I tested it on localnet and it works great 👍

image

Left a few commands but LGTM otherwise.

Also, could you please adapt:

  • x/supplier/config/supplier_configs_reader_test.go
  • x/tokenomics/keeper/token_logic_modules_test.go

To use uint32 too.

x/supplier/module/autocli.go Outdated Show resolved Hide resolved
x/supplier/keeper/query_supplier_test.go Outdated Show resolved Hide resolved
proto/poktroll/shared/service.proto Outdated Show resolved Hide resolved
@Olshansk
Copy link
Member Author

@red-0ne Appreciate the detailed review. Take a look at the changes I pushed. Thought you'd appreciate it :)

@Olshansk Olshansk requested a review from red-0ne January 17, 2025 00:53
Copy link
Contributor

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

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

So happy that we did not made compromises on the revshare precision 😍

Preemptively approving but PTAL at x/tokenomics/keeper/token_logic_modules_test.go

image

Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks.

You may need to run make trigger_ci to submit an empty commit that'll trigger the tests.

GCP workloads (requires changing the namespace to 1028)
Grafana network dashboard for devnet-issue-1028

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

@bryanchriswhite @red-0ne I'm going to merge this in to unblock work and keep moving, but wanted to call out this specific change that I'm making in the return queries:

  1. This hack:

    // TODO_MAINNET(@olshansk, #1033): Newer version of the CosmosSDK doesn't support maps.
    // Decide on a direction w.r.t maps in protos based on feedback from the CosmoSDK team.
    supplier.ServicesActivationHeightsMap = nil
  2. The test commented out in e2e/tests/stake_supplier.feature

  3. The ticket in [Cosmos] Revisit the encoding limitation of the newer version of cosmos #1033 where I'll keep posting updates, learnings and plans

@bryanchriswhite This is just an FYI, but feel free to review this after the fact and leave any additional comments. My goal was to not have your review being a blocker in getting this in.

@Olshansk Olshansk merged commit bb631a2 into main Jan 20, 2025
13 checks passed
@Olshansk Olshansk deleted the filter_suppliers_flag branch January 20, 2025 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devnet devnet-test-e2e on-chain On-chain business logic push-image CI related - pushes images to ghcr.io tooling Tooling - CLI, scripts, helpers, off-chain, etc...
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants