search

revault / revault-gui

Revault terminal
https://revault.dev
BSD 3-Clause "New" or "Revised" License
51 stars 8 forks source link

Manual CPFP #371

Open Zshan0 opened 2 years ago

Zshan0 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

https://github.com/revault/revault-gui/pull/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 :+1: .

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.