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

Feature: Add dry run CLI and use it in register_account_role #3992

Merged
merged 4 commits into from
Sep 19, 2023

Conversation

syan095
Copy link
Contributor

@syan095 syan095 commented Sep 13, 2023

Pull Request

Closes: PRO-807

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

Added a "dry run" CLI call that calls the substrate rpc, which dry runs a runtime call and returns the result.
Register account role will now do a dry run and return immediately if it runs into an error

dry runs a runtime call

Added a new dry run CLI function, which takes a RuntimeCall and returns the result.

Register account role will now do a dry run and return immediately if
it runs into an error
@linear
Copy link

linear bot commented Sep 13, 2023

PRO-807 Check account role before registering

In the LP Api, when we call lp_registerAccount, we should first query the state chain for the current account role.

If the role is already set, we should return an error with either "already registered as {account_role}".

@syan095 syan095 requested a review from kylezs September 13, 2023 01:59
@syan095
Copy link
Contributor Author

syan095 commented Sep 13, 2023

The code compiles, but I'm not sure what would be the best approach to test this. All info are welcome

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #3992 (5ca46b4) into main (7e48b0f) will decrease coverage by 0%.
The diff coverage is 0%.

@@          Coverage Diff          @@
##            main   #3992   +/-   ##
=====================================
- Coverage     72%     72%   -0%     
=====================================
  Files        367     367           
  Lines      58062   58081   +19     
  Branches   58062   58081   +19     
=====================================
- Hits       42028   42024    -4     
- Misses     13935   13955   +20     
- Partials    2099    2102    +3     
Files Changed Coverage Δ
api/lib/src/lib.rs 31% <0%> (-1%) ⬇️
...ne/src/state_chain_observer/client/base_rpc_api.rs 1% <0%> (-<1%) ⬇️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

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

@AlastairHolmes do you have any objections to adding this extension to the BaseRpcClient? Does this seem like the right approach?

api/lib/src/lib.rs Outdated Show resolved Hide resolved
@dandanlen dandanlen enabled auto-merge (squash) September 19, 2023 14:44
@dandanlen dandanlen merged commit 03aa032 into main Sep 19, 2023
44 checks passed
@dandanlen dandanlen deleted the feat/rpc-dry-run branch September 19, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants