-
Notifications
You must be signed in to change notification settings - Fork 15
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
Clean code, better documentation and comments #439
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for undertaking this large task, I have some comments that might be helpful going forward.
I will merge this PR as it is now, and distribute the remaining tasks accordingly. |
core/src/builder/transaction.rs
Outdated
create_btc_tx(tx_ins, vec![move_txout, anchor_output]) | ||
} | ||
|
||
/// Creates a [`TxHandler`] for the move_tx. | ||
/// Creates a [`TxHandler`] for the `move_to_vault_tx`. | ||
pub fn create_move_txhandler( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can change this name to move_to_vault_txhandler too. Also we can delete the function above that only creates tx after old code gets deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think create_move_to_vault_tx is needed since we only need the transaction and its id sometimes.
core/src/builder/transaction.rs
Outdated
@@ -651,6 +664,8 @@ pub fn create_operator_challenge_nack_txhandler( | |||
} | |||
} | |||
|
|||
/// Creates a [`TxHandler`] for the `operator_challenge_ACK_tx`. This transaction will allow the operator | |||
/// to send the `assert_begin_tx` to basically respond to the challenge(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe explain how. Like splitting their proof that they paid the withdrawal to parts and sending it to chain using winternitz keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
core/src/builder/transaction.rs
Outdated
@@ -1187,6 +1228,8 @@ pub fn create_reimburse_txhandler( | |||
} | |||
} | |||
|
|||
/// Creates a [`TxHandler`] for the `kickoff_timeout_tx`. This transaction will be sent by the operator | |||
/// if there are no challenges, to be able to send `reimburse_tx` later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kickoff timeout tx is sent if there was a challenge, but operator didnt create assert_end_tx in time (so they didn't send their proof in time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Description
This PR introduces better documentation and code comments. Also cleans the code.
Linked Issues
#410
#431
Testing
None
Docs
This whole PR is about the docs.