-
Notifications
You must be signed in to change notification settings - Fork 97
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
rpc: Add default config options to settings. #1981
Conversation
a454d9e
to
5cdd96c
Compare
Inserting the defaults in core would be less code and more strait forward, but web already has these values saved and in the forms... |
client/core/core.go
Outdated
// WalletDefinition gets the registered WalletDefinition for the asset and | ||
// wallet type. | ||
func walletDefinition(assetID uint32, walletType string) (*asset.WalletDefinition, error) { | ||
func WalletDefinition(assetID uint32, walletType string) (*asset.WalletDefinition, 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.
Kinda funky that this is a core
function. It only uses the client/asset
package internally, and it returns a client/asset
type.
I think we should move this to client/asset.WalletDefinition
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.
Moved. But the function name mirrors the type name, so the function renamed to WalletDef
611f2fa
to
78da726
Compare
If user does not supply some default values over rpc when creating wallets, use default values. Add the eth test token, zec, and doge to the simnet client set up script.
|
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.
LGTM, with the one noted caveat regarding the correct way to go from a configopt item to a string in the map[string]string
given to the wallet form.
// Apply default config options if they exist. | ||
for _, opt := range walletDef.ConfigOpts { | ||
if _, has := form.config[opt.Key]; !has && opt.DefaultValue != nil { | ||
form.config[opt.Key] = fmt.Sprintf("%v", opt.DefaultValue) |
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.
fmt.Sprintf("%v", ...)
should do the trick for all the types we give in DefaultValue
, but in our other uses of this field (WalletConfigForm.map
) we consider the other flags like isdate
and repeatable
. I don't feel like we need to replicate all that at this time though.
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.
To clarify, nothing to change unless another reviewer sees an issue related to this.
This is mostly to assist testing on simnet.