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

cli: Add support for address book #1087

Merged
merged 2 commits into from
Sep 16, 2022
Merged

cli: Add support for address book #1087

merged 2 commits into from
Sep 16, 2022

Conversation

matevz
Copy link
Member

@matevz matevz commented Aug 25, 2022

Fixes #892

  • adds addressbook command with subcommands add, rm, rename, list and show
  • stores native address and optionally ethereum address, if the address is a valid ethereum address
  • throws error, if withdrawing or transferring on consensus layer to a known account which is an ethereum account
  • makes importing and printing ethereum keys and addresses consistent

Example:

$ oasis addressbook add myaddr
? Address (bech32 or hex-encoded): [Enter 2 empty lines to finish]oasis1qq3agel5x07pxz08ns3d2y7sjrr3xf9paquhhhzl

? Address (bech32 or hex-encoded): 
oasis1qq3agel5x07pxz08ns3d2y7sjrr3xf9paquhhhzl

$ oasis addressbook show myaddr
Name:             myaddr
Native address:   oasis1qq3agel5x07pxz08ns3d2y7sjrr3xf9paquhhhzl

$ oasis addressbook add myethaddr
? Address (bech32 or hex-encoded): [Enter 2 empty lines to finish]0x90adE3B7065fa715c7a150313877dF1d33e777D5

? Address (bech32 or hex-encoded): 
0x90adE3B7065fa715c7a150313877dF1d33e777D5

$ oasis addressbook show myethaddr
Name:             myethaddr
Ethereum address: 0x90adE3B7065fa715c7a150313877dF1d33e777D5
Native address:   oasis1qpupfu7e2n6pkezeaw0yhj8mcem8anj64ytrayne

$ oasis addressbook list
LABEL                 ADDRESS                                        
myaddr                oasis1qq3agel5x07pxz08ns3d2y7sjrr3xf9paquhhhzl
myethaddr             0x90adE3B7065fa715c7a150313877dF1d33e777D5

$ oasis accounts transfer 10 myaddr --wallet test:alice --no-paratime
You are about to sign the following transaction:
Method: staking.Transfer
Body:
  To:     oasis1qq3agel5x07pxz08ns3d2y7sjrr3xf9paquhhhzl
  Amount: 10.0 TEST
Nonce:  28
Fee:
  Amount: 0.0 TEST
  Gas limit: 1266
  (gas price: 0.0 TEST per gas unit)

Account:  test:alice
Network:  testnet
Paratime: none (consensus layer)
? Sign this transaction? (y/N)

$ oasis accounts transfer 10 myethaddr --no-paratime
Unlock your account.
? Passphrase: 
Error: destination address named 'myethaddr' (0x90adE3B7065fa715c7a150313877dF1d33e777D5) will not be able to sign transactions on consensus layer

$ oasis accounts withdraw 10 myethaddr
Error: destination address named 'myethaddr' (0x90adE3B7065fa715c7a150313877dF1d33e777D5) will not be able to sign transactions on consensus layer

@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #1087 (785de04) into main (32815d9) will decrease coverage by 0.08%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #1087      +/-   ##
==========================================
- Coverage   67.88%   67.80%   -0.09%     
==========================================
  Files         131      131              
  Lines       11585    11584       -1     
==========================================
- Hits         7865     7855      -10     
- Misses       3688     3697       +9     
  Partials       32       32              
Impacted Files Coverage Δ
cli/wallet/file/file.go 2.62% <0.00%> (-0.07%) ⬇️
client-sdk/go/testing/testing.go 95.23% <ø> (ø)
client-sdk/go/helpers/address.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@matevz matevz force-pushed the matevz/feature/addressbook branch 3 times, most recently from 84f2f6c to ac68b57 Compare August 25, 2022 16:20
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Hm, I don't think that conflating this with the usual wallet accounts is a good idea. I would add a new configuration section called address book which is explicitly made just for storing addresses and then support this as an additional address resolution source.

@matevz matevz force-pushed the matevz/feature/addressbook branch 2 times, most recently from f56ea46 to 898308c Compare September 6, 2022 14:01
@matevz matevz requested a review from kostko September 6, 2022 14:02
@matevz matevz force-pushed the matevz/feature/addressbook branch 2 times, most recently from 199776b to 251e4dc Compare September 6, 2022 14:16
cli/addressbook/address.go Outdated Show resolved Hide resolved
cli/addressbook/address.go Outdated Show resolved Hide resolved
cli/cmd/addressbook.go Outdated Show resolved Hide resolved
cli/cmd/addressbook.go Outdated Show resolved Hide resolved
cli/addressbook/address.go Outdated Show resolved Hide resolved
cli/addressbook/address.go Outdated Show resolved Hide resolved
cli/cmd/common/wallet.go Outdated Show resolved Hide resolved
cli/cmd/wallet.go Outdated Show resolved Hide resolved
cli/config/config.go Outdated Show resolved Hide resolved
cli/addressbook/address.go Outdated Show resolved Hide resolved
cli/cmd/addressbook.go Outdated Show resolved Hide resolved
cli/cmd/addressbook.go Outdated Show resolved Hide resolved
cli/cmd/common/wallet.go Outdated Show resolved Hide resolved
cli/cmd/wallet.go Outdated Show resolved Hide resolved
cli/config/addressbook.go Outdated Show resolved Hide resolved
cli/config/addressbook.go Outdated Show resolved Hide resolved
cli/config/addressbook.go Outdated Show resolved Hide resolved
cli/config/addressbook.go Outdated Show resolved Hide resolved
cli/config/addressbook.go Outdated Show resolved Hide resolved
cli/cmd/wallet.go Outdated Show resolved Hide resolved
cli/config/addressbook.go Outdated Show resolved Hide resolved
cli/config/addressbook.go Outdated Show resolved Hide resolved
cli/cmd/addressbook.go Outdated Show resolved Hide resolved
cli/config/addressbook.go Outdated Show resolved Hide resolved
cli/config/addressbook.go Outdated Show resolved Hide resolved
cli/config/addressbook.go Outdated Show resolved Hide resolved
cli/config/addressbook.go Outdated Show resolved Hide resolved
cli/config/addressbook.go Outdated Show resolved Hide resolved
cli/addressbook/address.go Outdated Show resolved Hide resolved
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Almost there, left some minor comments and then it should be good to go!

cli/cmd/accounts.go Outdated Show resolved Hide resolved
cli/cmd/accounts.go Outdated Show resolved Hide resolved
cli/cmd/wallet.go Outdated Show resolved Hide resolved
cli/config/addressbook.go Outdated Show resolved Hide resolved
@matevz matevz force-pushed the matevz/feature/addressbook branch 2 times, most recently from 4f5af6e to 6a4cf44 Compare September 15, 2022 13:40
@matevz matevz changed the title cli: Add support for addressbook cli: Add support for address book Sep 16, 2022
@matevz matevz force-pushed the matevz/feature/addressbook branch 2 times, most recently from 52364e6 to dadcae3 Compare September 16, 2022 10:23
This enables labeling native oasis or ethereum addresses and using
them for transfers, withdrawals, deposits etc.
@matevz matevz merged commit 783afcc into main Sep 16, 2022
@matevz matevz deleted the matevz/feature/addressbook branch September 16, 2022 11:39
tjanez pushed a commit to oasisprotocol/cli that referenced this pull request Sep 20, 2022
tjanez pushed a commit to oasisprotocol/cli that referenced this pull request Oct 11, 2022
tjanez pushed a commit to oasisprotocol/cli that referenced this pull request Oct 11, 2022
tjanez pushed a commit to oasisprotocol/cli that referenced this pull request Oct 11, 2022
tjanez pushed a commit to oasisprotocol/cli that referenced this pull request Oct 11, 2022
tjanez pushed a commit to oasisprotocol/cli that referenced this pull request Oct 11, 2022
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.

cli: Add support for an address book
2 participants