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 fundrawtransaction command #1316

Merged
merged 3 commits into from
Jul 29, 2018

Conversation

vctt94
Copy link
Member

@vctt94 vctt94 commented Jun 27, 2018

@davecgh davecgh added this to the 1.3.0 milestone Jun 27, 2018
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.

While we will have less options than bitcoind, we should still use a single second parameter with a JSON object of optional parameters instead of making all of the options positional parameters.

@vctt94 vctt94 force-pushed the addFundrawtransaction branch 2 times, most recently from aed10ff to 20506e1 Compare July 12, 2018 13:31
@vctt94 vctt94 force-pushed the addFundrawtransaction branch from 20506e1 to 1bdf1cc Compare July 12, 2018 13:54
@vctt94
Copy link
Member Author

vctt94 commented Jul 12, 2018

I changed to use just one parameter, now I believe this PR is ready for another review.

@@ -240,6 +240,33 @@ func NewEstimatePriorityCmd(numBlocks int64) *EstimatePriorityCmd {
}
}

// FundRawTransactionOptions represents the optional inputs to fund
// a raw transaction.
type FundRawTransactionOptions struct {
Copy link
Member

Choose a reason for hiding this comment

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

This type must use json:"..." struct tags to provide the key name

// FundRawTransactionOptions represents the optional inputs to fund
// a raw transaction.
type FundRawTransactionOptions struct {
ChangeAccount *string
Copy link
Member

Choose a reason for hiding this comment

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

The change account is not part of the equivalent bitcoind RPC (instead, any address can be provided as an option). Why is this different?

// a raw transaction.
type FundRawTransactionOptions struct {
ChangeAccount *string
LockUnspents *bool `jsonrpcdefault:"false"`
Copy link
Member

Choose a reason for hiding this comment

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

Let's not add this for now. Locking outputs doesn't actually work in dcrwallet currently.

ChangeAccount *string
LockUnspents *bool `jsonrpcdefault:"false"`
FeeRate *float64
RequiredConfirmations *int32
Copy link
Member

Choose a reason for hiding this comment

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

bitcoind names this conf_target.

@vctt94
Copy link
Member Author

vctt94 commented Jul 20, 2018

fixed

@davecgh davecgh merged commit ea32a60 into decred:master Jul 29, 2018
JFixby pushed a commit to JFixby/dcrd that referenced this pull request Oct 21, 2018
@vctt94 vctt94 deleted the addFundrawtransaction branch April 10, 2019 12:22
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.

3 participants