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

Add CLI and library for extrinsics interaction #150

Merged
merged 38 commits into from
Jul 25, 2024
Merged

Conversation

jmg-duarte
Copy link
Contributor

@jmg-duarte jmg-duarte commented Jul 19, 2024

Description

Fixes #130

I've ended up introducing a separate CLI as I thought it would be weird to join two very separate sides in a single binary. If the team disagrees, I can put it in a single place.

The binary itself should be discoverable as is, but just --helping your way through.
Testing with the scenarios for the integration tests would be great to ensure this is working.

I'm looking for feedback and questions!

Important points for reviewers

subxt is a weird beast, take a look into the docs while reading the PR.

Keep in mind that some types needed wrappers or similar because of conversions to bounded vecs and friends.

Checklist

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

@jmg-duarte jmg-duarte requested review from cernicc and th7nder July 21, 2024 12:58
@jmg-duarte jmg-duarte marked this pull request as ready for review July 21, 2024 12:58
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.

you did a lot of evil magic! great work, let's iron out some intricacies and roll with it

I didn't run the code yet, will do that tomorrow -> wanna publish the findings of just reading the code for now.

cli/polka-storage/storagext/src/lib.rs Outdated Show resolved Hide resolved
cli/polka-storage/storagext/src/market.rs Outdated Show resolved Hide resolved
cli/polka-storage/storagext/src/lib.rs Outdated Show resolved Hide resolved
cli/polka-storage/storagext/src/market.rs Outdated Show resolved Hide resolved
cli/polka-storage/storagext/src/market.rs Show resolved Hide resolved
cli/polka-storage/storagext-cli/src/main.rs Outdated Show resolved Hide resolved
cli/polka-storage/storagext-cli/src/main.rs Outdated Show resolved Hide resolved
cli/polka-storage/storagext-cli/src/main.rs Outdated Show resolved Hide resolved
cli/polka-storage/storagext-cli/src/main.rs Show resolved Hide resolved
cli/polka-storage/storagext-cli/src/cmd/market.rs Outdated Show resolved Hide resolved
Copy link
Member

@cernicc cernicc left a comment

Choose a reason for hiding this comment

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

Wow. Good job figuring out the subxt 💪

  • I have a feeeling that at the end we might have a single cli endpoint for both provider and client. The polka-storage-provider might only be used for configuring and starting up the provider server and that is it.

  • Run taplo fmt before taging the pr. It will fail currently.

  • Do you have an idea how should we version the motadata.scale?

cli/polka-storage/storagext/src/market.rs Show resolved Hide resolved
cli/polka-storage/storagext/src/market.rs Show resolved Hide resolved
cli/polka-storage/storagext/src/lib.rs Outdated Show resolved Hide resolved
cli/polka-storage/storagext-cli/deals.json Outdated Show resolved Hide resolved
cli/polka-storage/storagext-cli/src/cmd/market.rs Outdated Show resolved Hide resolved
@jmg-duarte
Copy link
Contributor Author

I have a feeeling that at the end we might have a single cli endpoint for both provider and client. The polka-storage-provider might only be used for configuring and starting up the provider server and that is it.

We might, but the library at least was necessary and the current CLI is very cluttered already. Joining them later isn't too hard.

Do you have an idea how should we version the motadata.scale?

Honestly, no. We can just keep incrementing the name once someone makes a change, but since it's a binary blob it's really hard. The alternative is to codegen everything and check that in.

@jmg-duarte jmg-duarte added the ready for review Review is needed label Jul 22, 2024
cli/polka-storage/storagext-cli/src/main.rs Outdated Show resolved Hide resolved
cli/polka-storage/storagext-cli/src/main.rs Outdated Show resolved Hide resolved
cli/polka-storage/storagext-cli/src/main.rs Outdated Show resolved Hide resolved
cli/polka-storage/storagext-cli/src/cmd/market.rs Outdated Show resolved Hide resolved
cli/polka-storage/storagext-cli/src/main.rs Outdated Show resolved Hide resolved
@jmg-duarte jmg-duarte self-assigned this Jul 23, 2024
@jmg-duarte jmg-duarte added this to the Phase 1 milestone Jul 23, 2024
@jmg-duarte jmg-duarte added ready for review Review is needed and removed ready for review Review is needed labels Jul 23, 2024
cli/polka-storage/storagext-cli/README.md Outdated Show resolved Hide resolved
cli/polka-storage/storagext-cli/README.md Outdated Show resolved Hide resolved
cli/polka-storage/storagext-cli/README.md Outdated Show resolved Hide resolved
cli/polka-storage/storagext-cli/README.md Show resolved Hide resolved
cli/polka-storage/storagext-cli/README.md Outdated Show resolved Hide resolved
cli/polka-storage/storagext/src/lib.rs Outdated Show resolved Hide resolved
cernicc
cernicc previously approved these changes Jul 24, 2024
@jmg-duarte jmg-duarte added ready for review Review is needed and removed ready for review Review is needed labels Jul 24, 2024
@jmg-duarte jmg-duarte requested a review from th7nder July 24, 2024 12:52
zombienet/local-testnet.toml Outdated Show resolved Hide resolved
th7nder
th7nder previously approved these changes Jul 25, 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.

TREMENDOUSLY HUGE GREAT WORK!

cernicc
cernicc previously approved these changes Jul 25, 2024
Copy link
Member

@cernicc cernicc left a comment

Choose a reason for hiding this comment

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

Nice 💪 Great job!

@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels Jul 25, 2024
@jmg-duarte jmg-duarte dismissed stale reviews from cernicc and th7nder via ce25aeb July 25, 2024 08:43
@jmg-duarte jmg-duarte added ready for review Review is needed and removed ready for review Review is needed labels Jul 25, 2024
@jmg-duarte jmg-duarte enabled auto-merge July 25, 2024 09:00
@jmg-duarte jmg-duarte merged commit 8b34804 into develop Jul 25, 2024
3 checks passed
@jmg-duarte jmg-duarte deleted the feat/130/cli branch July 25, 2024 09:24
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.

CLI for the Parachain client side RPC
3 participants