-
Notifications
You must be signed in to change notification settings - Fork 161
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
Wallet CLI Implementation #1128
Conversation
all commands work except import, export (close), sign, and verify
Update: 7/11 commands working. Import, export, sign, verify are still WIP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on documentation. We need more of that. However, I noticed you're not using the rpc_api
fully. Let me know if you need help understanding how to best make use of it, however, the patterns should be complete enough in the other files that implement it in rpc_client
.
I noticed one additional issue after testing this. I would recommend making secp256k1 the default, like lotus. Otherwise, I tested out the new, list, and default commands, and they all worked great. I also double-checked that read permissions return a 403, while write permissions work as expected, and they all do, which is also good! The permissions list I added is working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one more thing you missed, after this, I think we're good. I've resolved the conversations that either have been addressed or will be done through additional rpc_api
refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested each command. Checked means it works:
- wallet default (without a wallet configured)
- wallet new
- wallet new secp256k1
- wallet new bls
- wallet has (tested wtih bls, secp256k1, and garbage addresses)
- wallet list
- wallet default (defaults to first wallet made)
- wallet set-default (tested with bls, secp256k1, and garbage addresses)
- garbage address causes unhandled panic in forest CLI (will make a separate issue)
FULLNODE_API_INFO="eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJBbGxvdyI6WyJyZWFkIiwid3JpdGUiLCJzaWduIiwiYWRtaW4iXX0.j5yGdcMgSAQd6raXLsryx-3I3fvL05QYh6BSKoNZtBA:/ip4/127.0.0.1/tcp/1234/http" forest wallet set-default f3va7krfg4svarx4uqfudab2z3bzbnjn6kipuahc5t3bp5jboowdnx2lj6vysbyaalkibeu7zhnala4sbku3gaasdf thread 'main' panicked at 'called
Result::unwrap()on an
Errvalue: InvalidLength', forest/src/cli/wallet_cmd.rs:160:63
- See: [Forest CLI] Fix panic when passing a malformed address to
wallet set-default
#1166
- wallet export (bls and secp256k1)
- wallet import (handles existing key)
- wallet sign -m 'f00f' -a (and verified the bytes differen than if signing with
-m 'f33f'
) - wallet verify - Fails with error decoding address. Will make a separate issue so it won't necessarily block merging this PR, since it doesn't break existing functionality. See: Fix
wallet verify
#1165
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor typos and things. As well as what @cryptoquick said. Not sure what happened... It was working last time we paired on it.... Though I think we should probably fix that before merging.
@ec2 I'm helping Connor get this across the finish line so it doesn't languish too long while he's got his current obligations to Uncle Sam. I've made changes based on your feedback. I'd like @connormullett to take on the two fixes in #1165 and #1166, but there's a lot of value in this PR already, and it doesn't break anything that existed before, I think? I'll leave it up to him to either fix it, or merge and fix via the tracking issue. |
Makes sense. Forgot about uncle sam. |
I merged it for Connor. I told him to take all the time he needs to address the remaining issues. It'll be good for me to build the net API on top of this, anyway. |
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
#537
#1118
Other information and links