-
Notifications
You must be signed in to change notification settings - Fork 15
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
Hardware wallet signer fixes #139 #253
Hardware wallet signer fixes #139 #253
Conversation
…AlDen_t139_hardware_wallet_signer ref fibercrypto#139
This reverts commit 533b214. ref fibercrypto#139
…let_signer Conflicts: .travis.yml Makefile vendor/github.com/skycoin/hardware-wallet-go/src/skywallet/messages.go vendor/github.com/skycoin/hardware-wallet-go/src/skywallet/skywallet.go vendor/github.com/skycoin/hardware-wallet-protob/go/google/protobuf/descriptor.pb.go vendor/github.com/skycoin/hardware-wallet-protob/go/messages.pb.go vendor/github.com/skycoin/hardware-wallet-protob/go/types.pb.go vendor/golang.org/x/sys/windows/syscall_windows.go vendor/golang.org/x/sys/windows/zsyscall_windows.go ref fibercrypto#139
address index for the spend output related for the input is used to find the priv/pub key pair ref fibercrypto#139
…hardware_wallet_signer Conflicts: Gopkg.lock ref fibercrypto#139
… GetLoadedAddresses ref fibercrypto#139
@@ -812,7 +816,31 @@ func NewWalletDirectory(dirPath string) *WalletDirectory { | |||
type WalletDirectory struct { | |||
// Implements WallentEnv interface | |||
WalletDir string | |||
wltService *SkycoinLocalWallet | |||
wltService core.PersistibleSet |
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.
@stdevAlDen why this change ?
"github.com/fibercrypto/fibercryptowallet/src/coin/skycoin/params" | ||
"github.com/fibercrypto/fibercryptowallet/src/coin/skycoin/skytypes" | ||
"github.com/fibercrypto/fibercryptowallet/src/core" | ||
fce "github.com/fibercrypto/fibercryptowallet/src/errors" |
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.
fce
=> fcwerrors
return inputs, nil | ||
} | ||
|
||
func spendingOutputFromRemote(inputHash cipher.SHA256) (*readable.SpentOutput, error) { |
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.
Duplicated . Reuse the logic in Skycoin altcoin plugin .
return | ||
} | ||
|
||
func readableTxn2Transaction(txn skytypes.ReadableTxn) (*coin.Transaction, error) { |
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.
Duplicated . Reuse Skycoin altcoin plugin code .
|
||
func rawTxn2Transaction (txn skytypes.SkycoinTxn) (*coin.Transaction, error) { | ||
buf, err := txn.EncodeSkycoinTransaction() | ||
if err != nil { |
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.
What happens on error ?
@@ -0,0 +1,428 @@ | |||
package hardware |
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.
Package name should match folder name . skywallet
?
@@ -24,6 +24,11 @@ type WalletSet interface { | |||
SupportedWalletTypes() []string | |||
} | |||
|
|||
type PersistibleSet interface { |
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.
Need be renamed .
@@ -618,6 +618,10 @@ func (walletM *WalletManager) signTxn(wltIds, address []string, source string, t | |||
logWalletManager.WithError(err).Warnf("No signer %s for wallet %v", source, wlts[0]) | |||
return nil | |||
} | |||
if suid, err := signer.GetSignerUID(); err != nil && wlts[0].GetId() == string(suid) { |
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.
condition should be err == nil
, right ?
} | ||
return false | ||
} | ||
// FIXME: consider a double checking here instead of hadHwConnected |
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.
What about this FIXME
tag ? Shall this remain ?
@@ -0,0 +1,23 @@ | |||
package util |
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.
Shouldn't this be under skywallet
contrib package ?
…let_signer' into stdevAlDen_t139_hardware_wallet_signer
…Timeout vs SkycoinBlockchain.CacheTime
…freshTimeout and SkycoinBlockchain.CacheTime
…g signature of GetSpentOutput and GetAddress ... still WiP
Fixes #139
Changes:
vendor/github.com/SkycoinProject/skycoin
git submodule, use it from vendors.GetInputs
, previous cached value.EnumerateSignServices
for local/remote wallet.TestTransactionSignInputFromDevice
for the hardware wallet signer.QSigner
andSignerModel
modelsDoes this change need to mentioned in CHANGELOG.md?
yes
Requires changes in protobuf specs?
no
Requires testing
yes