-
Notifications
You must be signed in to change notification settings - Fork 113
fix(zcoin): add locked notes support for withdrawals #2644
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
base: dev
Are you sure you want to change the base?
Conversation
…sactionRes->SignRawTransactionRes
* dev: fix(TPU): correct dexfee in check balance to prevent swap failures (#2600) fix(tests): fix/remove kmd rewards failing test (#2633) chore(ci): bump CI container image to debian bullseye-slim to match dev (#2641) chore(release): add changelog entries for v2.5.2-beta (#2639) chore(release): bump mm2 version to 2.5.2-beta (#2638) feat(ci): add macos universal2 build (#2628) fix(metrics): remove memory_db size metric (#2632) chore(rust 1.90): make CI clippy/fmt pass Revert "fix(ordermatch): ignore loop-back; clear on null root; reject stale keep-alives (#2580)" Revert "fix(orderbook): validate roots before commit (#2605)"
| impl ZCoin { | ||
| #[cfg(feature = "run-docker-tests")] | ||
| pub async fn withdraw_for_tests(&self, req: WithdrawRequest) -> Result<TransactionDetails, MmError<WithdrawError>> { | ||
| if req.fee.is_some() { | ||
| return MmError::err(WithdrawError::UnsupportedError( |
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 the reason for not using the other init_withdraw/status_withdraw is that it's task managed and need an rpc and a MarketMakerIt to start?
can't we call init_withdraw here and provide a dummy task_handle just for processing this task and polling the status?
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.
Yes, I'd like to.
I remember in the past I once tried to create a dummy task handle but that would have required too much extra code and some task manager refactoring. I was not sure it was worth it then and now I did not go that way too.
| /// Request to send zcoin raw transaction with extra parameters | ||
| #[derive(Clone, Debug, Deserialize)] | ||
| pub struct ZcoinSendRawTransactionRequest { | ||
| coin: String, | ||
| /// List of rseeds used to create transaction notes, needed to lock sent notes properly | ||
| pub rseeds: Vec<String>, | ||
| /// Change amount | ||
| pub received_by_me: BigDecimal, | ||
| /// Serialized zcoin transaction | ||
| pub tx_hex: String, | ||
| } |
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.
Can we put this in a better place, like RPC or ZCoin crates?
| /// List of rseeds, for Zcoin | ||
| pub rseeds: Option<Vec<String>>, |
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.
Why is it Optional?
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.
This TransactionDetails struct is common for all coin types, so we just use Option for those fields which may be relevant only for some coin type
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.
Do we encode this type anywhere? If not, we should make it generic enum that takes protocol-specific TransactionDetails (not in this PR). cc @shamardy
|
|
||
| /// Request to send zcoin raw transaction with extra parameters | ||
| #[derive(Clone, Debug, Deserialize)] | ||
| pub struct ZcoinSendRawTransactionRequest { |
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.
Typo fix:
| pub struct ZcoinSendRawTransactionRequest { | |
| pub struct ZCoinSendRawTransactionRequest { |
|
@shamardy |
|
@shamardy |
|
@shamardy |
|
@shamardy |
|
@shamardy |
1 similar comment
|
@shamardy |
|
@shamardy |
5 similar comments
|
@shamardy |
|
@shamardy |
|
@shamardy |
|
@shamardy |
|
@shamardy |
Past PR #2331 added support for unconfirmed notes handling to prevent spent notes reuse and show unconfirmed change notes as unspendable balance. To implement that, PR 2331 added a locked notes DB.
However, that PR added support only for swap transactions, so if a withdrawal is called before or during a swap (while spent notes are not confirmed yet) errors like
18: bad-txns-joinsplit-requirements-not-metmay occur when withdrawal and swap transactions are sent to the chain.This PR adds support for the locked notes DB for zcoin withdrawals as well. The
init_withdrawRPC now returns array ofrseedsvalues which are used to access the locked notes DB. Also the init_withdraw RPC returnsreceived_by_medecimal amount which is used to store change value in the locked notes DB. Both rseeds and received_by_me values should be passed along with the transaction in hex to the RPC sending the transaction into the network.Breaking change
To send zcoin transaction into the network a new
z_send_raw_transactionRPC must be used, instead of the old "send_raw_transaction". Thez_send_raw_transactionRPC has more parameters:The "rseeds", "received_by_me", "tx_hex" fields are returned by
init_withdrawRPC.TODO:
Will we ever support ZIP 32? For future support we could add
AccountIdfield into the locked notes DB.