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

Manual CPFP #371

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Manual CPFP #371

wants to merge 2 commits into from

Conversation

Zshan0
Copy link
Contributor

@Zshan0 Zshan0 commented Jul 2, 2022

I started working on the gui side of the manual CPFP command implementation. Currently, I'm working on the SpendTransaction portion of it. The UI will be the same on the unvault but since broadcast already existed for SpendTransaction I decided to start from it. I have a few questions before I proceed.

One last thing, suppose I want to run my local revaultd along with local revault-gui on aquarium how do I do that?

@Zshan0
Copy link
Contributor Author

Zshan0 commented Jul 3, 2022

There is another issue that I am facing, I am not sure when to construct the CPFP variant of SpendTransaction under which I have implemented it since I assumed that CPFP too is an operation for SpendTransaction.

Currently the final state of SpendTransactionAction view is SharePsbt. I am not sure what should be the state where I will return CPFP

@edouardparis
Copy link
Member

How can I check if the transaction can be CPFPed, can all spend transactions be CPFPed?

SpendTx that can be cpfped can only be SpentTX with ListSpendTxStatus equals to ListSpendTxStatus::Broadcasted

How should I throw errors for incorrect format of feerate, I looked at psbt from SharePsbt and I have also seen such error handling

The form::Value has a field valid, that you should set as false if the given value cannot be parse as string.
Then you add this form::Value to a form::Form in the view with a simple error message as string
ex: https://github.com/revault/revault-gui/blob/master/src/app/view/manager.rs#L403
I know it is not very clean, but iced text_input can only returns String and I did not create a new widget for numeric input yet.

#369, not sure which one should I follow.
In https://github.com/revault/revault-gui/compare/master...Zshan0:revault-gui:manual_cpfp?expand=1#diff-a5ee58847c74b9596f114a63e0133f107b597d19cb398e24902c5829c5624a51 you can see that I have commented out the actual call because it is not able to detect such method exists in revaultd.

If you want to integrate your revaultd PR in the GUI you can change Cargo.toml with
revaultd = {git="github.com/Zshan0/revaultd" branch="jsonrpc_test", default-features = false}

One last thing, suppose I want to run my local revaultd along with local revault-gui on aquarium how do I do that?

I guess you can do it like this
In revaultd repo: cargo run -- --config <aquarium-path>/<demo>/revaultd-man-0/config.toml

@Zshan0
Copy link
Contributor Author

Zshan0 commented Jul 22, 2022

After further studying, I think it is best to not treat CPFP as a SpendTransactionAction but treat it separately and common for all the transactions. But the view will be only rendered to the ones that can be CPFPed. I will create two modules SpendTransactionCPFP and UnvaultTransactionCPFP (Although I am not sure if it will be suitable for Unvault transactions since there is no UnvaultTransactionAction).

I am not creating a common module for both of these because the way it is triggered for spend and unvault will be different. unvault transactions will be CPFPed in a batch where as spend transactions for now will be CPFPed individually.

Please let me know if there are any comments/fixes that I must do

@Zshan0
Copy link
Contributor Author

Zshan0 commented Jul 22, 2022

Since the feerate is taken in sat/vbyte I will not be able to use bitcoin::Denomination since I believe it does not have this unit, so I am not sure what unit to use instead.

@edouardparis
Copy link
Member

If I understand correctly, It is better in your opinion to redirect the user to a special dedicated panel than trying to integrate the action inside the spend panel. I am not opposed to this idea if this help to keep the code simple 👍 .

Since the feerate is taken in sat/vbyte I will not be able to use bitcoin::Denomination since I believe it does not have this unit, so I am not sure what unit to use instead.

You can directly display the sat/vbyte without using the conversion.rs or bitcoin::Denomination.

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