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

cgo: Add create signed transaction. #12

Merged
merged 1 commit into from
Mar 27, 2024
Merged

cgo: Add create signed transaction. #12

merged 1 commit into from
Mar 27, 2024

Conversation

JoeGruffins
Copy link

@JoeGruffins JoeGruffins commented Feb 2, 2024

closes #11

Depends on all the other stuff atm.

Wallet logic taken from dcrdex.

@JoeGruffins JoeGruffins changed the base branch from main to cgo February 2, 2024 13:50
@JoeGruffins JoeGruffins marked this pull request as ready for review February 5, 2024 08:28
Amount uint64
}

func (w *Wallet) SignSendTransaction(ctx context.Context, outputs []*Output, feeRate uint64) (signedTx []byte, txid *chainhash.Hash, fee uint64, err error) {
Copy link
Owner

Choose a reason for hiding this comment

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

My understanding is that this method is supposed to create a signed tx that auto selects inputs to send funds to the specified outputs. I think it can be less complex if we follow the pattern in the dcrwallet jsonrpc fundRawTransaction method.

Basically, we just need to call w.NewUnsignedTransaction.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, the method name is a little complex / confusing. CreateSignedTransaction?

Copy link
Author

Choose a reason for hiding this comment

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

I think we will want to give an option to select inputs in this first iteration, so we will need the complexity anyhow. Let me double check.

@JoeGruffins
Copy link
Author

@JoeGruffins
Copy link
Author

Greatly simplified and able to pick inputs https://github.com/itswisdomagain/libwallet/compare/793fe5ef8597787a6da7e9882b3efea9b6823f4b..ca1b6ac9907942e187aacc7260af4aae51085511

@JoeGruffins JoeGruffins changed the title cgo: Add sign send transaction. cgo: Add create signed transaction. Feb 15, 2024
@JoeGruffins
Copy link
Author

Sorry changing once more. We can also have frozen utxo.

@JoeGruffins
Copy link
Author

Now we can specify inputs to ignore as well. Looks messy again. Lightly tested.

@JoeGruffins
Copy link
Author

cgo/types.go Outdated Show resolved Hide resolved
Comment on lines 155 to 208
// NOTE: This is not instantaneous and may take a few moments to send. Avoid
// closing the wallet directly after sending.
Copy link
Owner

Choose a reason for hiding this comment

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

Should we add a delay in this method? I think if we don't, we have no way of preventing the user from closing the wallet immediately after a broadcast.

Copy link
Author

@JoeGruffins JoeGruffins Mar 6, 2024

Choose a reason for hiding this comment

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

I tested some more today and the wallet will send the tx again on the next start up. Because we don't have access to a dcrd with this info stored (mempool) I don't think we can know if it made it out onto the network unless we wait for a confirmation. So... no I think its fine. There's nothing we can know for certain without waiting too long.

Copy link
Author

Choose a reason for hiding this comment

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

I'm going to remove the note.

asset/dcr/transactions.go Show resolved Hide resolved
asset/dcr/transactions.go Outdated Show resolved Hide resolved
asset/dcr/transactions.go Outdated Show resolved Hide resolved
Comment on lines +151 to +172
inputSource = func(target dcrutil.Amount) (detail *txauthor.InputDetail, err error) {
for _, utxo := range unspents {
if details.Amount >= target {
break
}
coinID := fmt.Sprintf("%s:%d", utxo.TxID, utxo.Vout)
if _, ignore := ignoreCoinIDs[coinID]; ignore {
continue
}
if !utxo.Spendable {
continue
}
if err := addUTXO(utxo, coinID); err != nil {
return nil, err
}
}
return details, nil
}
Copy link
Owner

Choose a reason for hiding this comment

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

The other inputSource doesn't call addUTXO. It adds the utxos early on and simply returns details. Not sure which is better, they're likely the same thing but it'd aid consistency if both do the same thing. I'm leaning towards preparing the details outside of the inputSource fn. And that makes me wonder if it is possible to have a different fn prepare and return the details, so this one is less congested.

Copy link
Author

@JoeGruffins JoeGruffins Mar 7, 2024

Choose a reason for hiding this comment

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

I think I put it inside so that that we don't need to worry about calculating the fee. We can use the amount that dcrwallet calculates. The one where we do it outside just uses all the inputs and will error if it's not enough so not worrying about the amount at all.

Copy link
Author

Choose a reason for hiding this comment

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

Added a couple comments.

Copy link
Author

Choose a reason for hiding this comment

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

I guess it wouldn't be much work to estimate the fee, if you think we should. We could just copy whatever wallet is doing to make sure the amounts aren't different.

go.mod Outdated Show resolved Hide resolved
cgo/transactions.go Outdated Show resolved Hide resolved
cgo/transactions.go Outdated Show resolved Hide resolved
@JoeGruffins
Copy link
Author

@itswisdomagain itswisdomagain merged commit 935a436 into itswisdomagain:cgo Mar 27, 2024
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.

cgo: Add transaction signing and sending.
2 participants