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

Feat/66/polka storage provider print start time of the provider #86

Merged

Conversation

cernicc
Copy link
Member

@cernicc cernicc commented Jun 20, 2024

Description

Implemented rpc client for the storage provider. The usage can be seen when calling info command. The client leverages the existing trait used for defining rpc methods.

How to test

Run rpc server: RUST_LOG=polka_storage_provider=trace cargo run --bin polka-storage-provider -- run --rpc-address ws://127.0.0.1:<parachain_node_port>
Execute cli info command: RUST_LOG=polka_storage_provider=trace cargo run --bin polka-storage-provider -- info

Checklist

  • Are there important points that reviewers should know?
    • If yes, which ones?
  • Make sure that you described what this change does.
  • If there are follow-ups, have you created issues for them?
    • If yes, which ones? / List them here
  • Have you tested this solution?
  • Were there any alternative implementations considered?
  • Did you document new (or modified) APIs?

@cernicc cernicc linked an issue Jun 20, 2024 that may be closed by this pull request
@cernicc cernicc self-assigned this Jun 20, 2024
@cernicc cernicc added the ready for review Review is needed label Jun 20, 2024
@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels Jun 20, 2024
Copy link
Contributor

@th7nder th7nder left a comment

Choose a reason for hiding this comment

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

Looks good, could you provide some instructions in the PR how can I run it locally and test it?

cli/polka-storage-provider/src/rpc/client.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/client.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/client.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/client.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/client.rs Outdated Show resolved Hide resolved
@cernicc cernicc requested a review from th7nder June 21, 2024 09:54
Copy link
Contributor

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

I struggle to understand some decisions, comments would definitely help, though I still left some questions looking for some insight!

cli/polka-storage-provider/src/rpc/client.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/client.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/client.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/client.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/client.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/client.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/client.rs Outdated Show resolved Hide resolved
@cernicc cernicc requested a review from jmg-duarte June 27, 2024 09:20
Copy link
Contributor

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

Great work! The API feels more natural now.

I've left some nitpicks and requests.

A quick note on them so you know where I'm coming from: after suffering a lot with Filecoin's lack of clarity and documentation, I believe we ought to be better and make code more readable without requiring you to already be a developer of the codebase. It helps anyone doing reviews as well as help onboard anyone that joins the project, saving everyone time in the process.

cli/polka-storage-provider/src/commands/run.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/commands/run.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/commands/runner.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/version.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/version.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/client.rs Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/server.rs Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/server.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/methods.rs Outdated Show resolved Hide resolved
cli/polka-storage-provider/src/rpc/client.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

LGTM, great job!

@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels Jun 27, 2024
@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels Jun 28, 2024
@cernicc cernicc merged commit ee54b73 into develop Jun 28, 2024
3 checks passed
@cernicc cernicc deleted the feat/66/polka-storage-provider-print-start-time-of-the-provider branch June 28, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Review is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Polka storage provider: Print start time of the provider
3 participants