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

dcrjson: Add SweepAccountCmd & SweepAccountResult. #1027

Merged
merged 1 commit into from
Feb 12, 2018

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented Feb 12, 2018

No description provided.

Copy link
Member

@jrick jrick left a comment

Choose a reason for hiding this comment

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

In addition to these requested changes, there should also be tests for encoding and decoding the requests


// SweepAccountCmd defines the sweep account JSON-RPC command.
type SweepAccountCmd struct {
SourceAccount uint32
Copy link
Member

Choose a reason for hiding this comment

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

the json-rpc api uses account names (strings) to refer to accounts

type OutputDestination struct {
Address string
Script []byte
ScriptVersion uint32
Copy link
Member

Choose a reason for hiding this comment

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

we probably don't need the script and script version, and can just work with an address string instead of this type. P2SH is a better idea anyways for using custom scripts

type SweepAccountCmd struct {
SourceAccount uint32
RequiredConfirmations uint32
FeePerKb dcrutil.Amount
Copy link
Member

Choose a reason for hiding this comment

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

since json-rpc request parameters are positional, and we want the required confirmations and fee to be configurable but use the wallet defaults, these two must come last.

Additionally, the json-rpc api uses json numbers between 0-21M when referring to amounts, so this must be a float64 (we should not be importing dcrutil at all here)

SourceAccount uint32
RequiredConfirmations uint32
FeePerKb dcrutil.Amount
ChangeDestination *OutputDestination
Copy link
Member

Choose a reason for hiding this comment

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

Is ChangeDestination where the account is swept to? I'm assuming so, but this needs a different name. Perhaps just DestinationAddress string (see comments above about not needing the scripts).

}

// NewSweepAccountCmd returns a new instance which can be used to issue a JSON-RPC SweepAccountCmd command.
func NewSweepAccountCmd(sourceAccount uint32, requiredConfs uint32, feePerKb *dcrutil.Amount, changeDestination *OutputDestination) *SweepAccountCmd {
Copy link
Member

Choose a reason for hiding this comment

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

after reordering the request parameters in the struct, these should be reordered as well.

@@ -5,6 +5,8 @@

package dcrjson

import "github.com/decred/dcrd/dcrutil"
Copy link
Member

Choose a reason for hiding this comment

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

no dcrutil import, use float64 for amounts

// SweepAccountResult models the data returned from the sweepaccount
// command.
type SweepAccountResult struct {
UnsignedTransaction []byte `json:"unsignedTransaction"`
Copy link
Member

Choose a reason for hiding this comment

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

binary data must be a hexadecimal string in the json-rpc api

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, the json struct tags must use all lowercase for consistency with the rest of the api.

@dnldd dnldd force-pushed the sweepaccount branch 3 times, most recently from 29fbc2b to 6a461c8 Compare February 12, 2018 20:58
@jrick jrick merged commit 0bc305f into decred:master Feb 12, 2018
@dnldd dnldd deleted the sweepaccount branch June 4, 2018 23:42
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.

2 participants