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(wallet): add grpc method for setting base node #3828

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

zhangcheng
Copy link
Contributor

@zhangcheng zhangcheng commented Feb 13, 2022

Description

Aim to resolve #3821

Motivation and Context

Learning via "good first issue"

How Has This Been Tested?

Code compiles, and manual test with grpcurl.

@delta1 delta1 changed the title try to fix #3821 (WIP) feat(wallet): add grpc method for setting base node Feb 13, 2022
Copy link
Contributor

@delta1 delta1 left a comment

Choose a reason for hiding this comment

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

Thanks @zhangcheng looks good! You will just have to run cargo fmt --all to pass clippy CI.

Regarding testing:

  • manually: use a gRPC client to to call against your method with the wallet running
  • automated: take a look at integration_tests folder (but you can leave this out for now)

@zhangcheng
Copy link
Contributor Author

Got it, thx @delta1 , will clippy it.

Copy link
Contributor

@philipr-za philipr-za 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, will approve once Clippy passes.

@zhangcheng
Copy link
Contributor Author

I tried to use grpcurl to test against it, but my limited knowledge couldn't help me to construct valid inputs.
It'd be very helpful if there is a brief guide or alike to educate newbies like me on how to properly construct needed inputs with the correct format.

grpcurl -plaintext \
    -import-path ./applications/tari_app_grpc/proto \
    -proto wallet.proto \
    -d '{"public_key": "36a9df45e1423b5315ffa7a91521924210c8e1d1537ad0968450f20f21e5200d", "net_address": "localhost"}' \
    localhost:18143 \
    tari.rpc.Wallet.SetBaseNode

@philipr-za
Copy link
Contributor

I tried to use grpcurl to test against it, but my limited knowledge couldn't help me to construct valid inputs. It'd be very helpful if there is a brief guide or alike to educate newbies like me on how to properly construct needed inputs with the correct format.

grpcurl -plaintext \
    -import-path ./applications/tari_app_grpc/proto \
    -proto wallet.proto \
    -d '{"public_key": "36a9df45e1423b5315ffa7a91521924210c8e1d1537ad0968450f20f21e5200d", "net_address": "localhost"}' \
    localhost:18143 \
    tari.rpc.Wallet.SetBaseNode

I haven't personally tested a CLI gRPC client at all, I prefer a GUI client like BloomRPC which can just ingest the proto file. from a glance that request looks ok except for the format of the net_address field. We are using the self describing MultiAddr which would look like /ip4/127.0.0.1.

@delta1
Copy link
Contributor

delta1 commented Feb 14, 2022

I agree with @philipr-za it's much easier to test with a GUI. But not a showstopper here, if the CI passes then LGTM

stringhandler
stringhandler previously approved these changes Feb 14, 2022
@stringhandler
Copy link
Collaborator

The address can be /ip4/127.0.0.1/tcp/18142

@stringhandler
Copy link
Collaborator

Just need to sign your commits

@zhangcheng
Copy link
Contributor Author

ERROR:
Code: InvalidArgument
Message: Base node public key was not a valid pub key: The input data was the incorrect length to perform the desired conversion

Since I don't know how to construct a valid public key string for this case yet, the above string was pulled out of the log files, such as:

2022-02-14 12:00:06.557504000 [comms::node] [Thread:4469540224] INFO Your node's public key is '1ed8d6e659bdabc5bd1f7cb6f8cfbbdbd97f5413fb8c40fc35fcc4cccb54df69'

@stringhandler
Copy link
Collaborator

The grpc client often wants bytes to be base64, so you can try HtjW5lm9q8W9H3y2+M+729l/VBP7jED8NfzEzMtU32k=

@zhangcheng
Copy link
Contributor Author

Just need to sign your commits

Something new for me to learn then. 😄

@zhangcheng
Copy link
Contributor Author

Cool, base64 string works. Thanks @mikethetike 🤝

grpcurl -plaintext \
    -import-path ./applications/tari_app_grpc/proto \
    -proto wallet.proto \
    -d '{"public_key": "HtjW5lm9q8W9H3y2+M+729l/VBP7jED8NfzEzMtU32k=", "net_address": "/ip4/127.0.0.1/tcp/18142"}' \
    localhost:18143 \
    tari.rpc.Wallet.SetBaseNode

Console output:

Tari Console Wallet running... (gRPC mode started)
Setting base node peer...
1ed8d6e659bdabc5bd1f7cb6f8cfbbdbd97f5413fb8c40fc35fcc4cccb54df69::/ip4/127.0.0.1/tcp/18142

@delta1
Copy link
Contributor

delta1 commented Feb 14, 2022

Well done 👏

delta1
delta1 previously approved these changes Feb 14, 2022
Description
---
Aim to resolve tari-project#3821

Motivation and Context
---
Learning via "good first issue"

How Has This Been Tested?
---
Manual test with `grpcurl`
@zhangcheng
Copy link
Contributor Author

I haven't personally tested a CLI gRPC client at all, I prefer a GUI client like BloomRPC which can just ingest the proto file.

Just want to say, BloomRPC is much nicer and easier to use than grpcurl, esp. to a newbie. @philipr-za

Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

I preferred the binary version, but this is fine too.

@stringhandler
Copy link
Collaborator

I've queued it. If the CI passes, it will get merged. Thanks so much!

@aviator-app aviator-app bot merged commit 8791e93 into tari-project:development Feb 14, 2022
@zhangcheng zhangcheng deleted the to_fix_#3821 branch February 14, 2022 15:30
sdbondi added a commit to sdbondi/tari that referenced this pull request Feb 17, 2022
* development: (28 commits)
  fix(wallet): fix aggressive disconnects in wallet connectivity (tari-project#3807)
  chore: honor decimals in ERC20 (tari-project#3809)
  chore: add icons to applications (tari-project#3812)
  fix: daily test (tari-project#3815)
  ci: improvements to macos pkg (tari-project#3824)
  ci: cargo test speedups (tari-project#3843)
  feat: add persistence of transaction cancellation reason to wallet db (tari-project#3842)
  fix: update RFC links and README (tari-project#3675) (tari-project#3839)
  chore: change NodeIdentity debug (tari-project#3817)
  fix(dan): include state_root in node hash (tari-project#3836)
  feat(cli): resize terminal height (tari-project#3838)
  feat(validator-node): initial state sync implementation (partial) (tari-project#3826)
  feat: update console wallet tui (tari-project#3837)
  feat: resize base node terminal on startup (tari-project#3827)
  feat(wallet): add grpc method for setting base node (tari-project#3828)
  chore(deps): bump follow-redirects from 1.14.5 to 1.14.8 in /applications/tari_web_extension_example (tari-project#3832)
  chore(deps): bump follow-redirects from 1.14.7 to 1.14.8 in /applications/tari_collectibles/web-app (tari-project#3833)
  chore(deps): bump follow-redirects from 1.14.5 to 1.14.8 in /applications/tari_web_extension (tari-project#3834)
  chore(deps): bump follow-redirects from 1.14.7 to 1.14.8 in /applications/launchpad/gui-vue (tari-project#3831)
  chore(deps): bump follow-redirects from 1.14.4 to 1.14.8 in /integration_tests (tari-project#3829)
  ...
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.

Create a gRPC interface in console wallet to set the connected base node
4 participants