Conversation
- These don't exist for wallet. 'cargo test' seems like the only thing that exists
- Comments expanded to indicate stateless nature of function
- We can now call 'epic-wallet cancel -i <x> -m epicbox' for a given tx, and if an epicboxmsgid is bound, it will execute - We can also call 'epic-wallet cancel -e <epicbox_msg_id>'
merge changes from master into ebox-cancel-txs
- CancelTx also calls this persistent ID, which is now passed through to other epicboxes
- epicbox_tx_id was not persisting properly on mobile
who-biz
left a comment
who-biz
left a comment
There was a problem hiding this comment.
Review comments mostly for my own notes.
| Some(&m) => Some(m.to_owned()), | ||
| }; | ||
|
|
||
| self.tx_lock_outputs(keychain_mask, &slate, 0, Some(sa.dest))?; |
There was a problem hiding this comment.
Pretty sure we need to lock before send with the new mechanics, but I will also test once more to be sure. New mechanics benefit from the lock, since we add an EpicboxTxId in the epicbox_channel.send() call below.
| epicbox_tx_id | ||
| } | ||
|
|
||
| (None, Some(id), None) => { |
There was a problem hiding this comment.
This function body needs cleaned up. We should probably allow callers to use both id and epicboxtxid simultaneously, similar to slate uuid usage below.
| keychain_mask, | ||
| |api, m| { | ||
| api.set_epicbox_config(epicbox_config.clone()); | ||
| // If error, nothing was cancelled anywhere the wallet can verify. |
There was a problem hiding this comment.
This comment is now incorrect. Local cancellation fallback happens in adapters/epicbox.rs function body, for backwards compat with epicbox servers <= v3.0.0
|
|
||
| match (major, minor, patch) { | ||
| (Some(major), Some(minor), Some(patch)) => { | ||
| (major, minor, patch) >= (3, 1, 0) |
There was a problem hiding this comment.
Should make this a standard version comparison function, not specifically limited to 3.1.0 and epicbox tx cancellation. We will have more extensions in future to Epicbox Protocol
| _ => false, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
TODO: document new version check mechanics. We now send ClientDetails and expect relay to respond with a version. Or we timeout.
New epicbox servers will respond with a version. <= 3.0.0 will not.
Additionally, EpicboxBroker has been overhauled completely to handle multiple events rather than trying to infer.
| epicboxtxid: epicboxtxid, | ||
| }); | ||
|
|
||
| if wallet_mode != "send" { |
There was a problem hiding this comment.
TODO: We should change this wallet_mode to something more descriptive than send which is a string type (type also should be changed).
We now have two separate "single shot send" style protocol messages (i.e. 1 - send, and 2 - cancel).
| epicboxmsgid, | ||
| epicboxtxid, | ||
| } => { | ||
| let returned_epicboxtxid = if let Some(value) = epicboxtxid { |
There was a problem hiding this comment.
Note: this is another area where the evaluation of EpicboxTxId::parse() outcomes - within a dedicated EpicboxTxId helper - can simplify this file.
(We can then eliminate all of the if let Some(value) + nested match
| } | ||
| }; | ||
|
|
||
| info!( |
There was a problem hiding this comment.
TODO: consider reducing spammy info logging? Might be helpful for transition period, however.
| } | ||
| } | ||
|
|
||
| Message::Binary(bytes) => { |
There was a problem hiding this comment.
Message::Binary has been moved to here. Need to figure out why this was there in the first place honestly.
See: https://github.com/EpicCash/epic-wallet/pull/125/changes#r3918572935
| ProtocolResponseV2::Ok { | ||
| epicboxmsgid, | ||
| epicboxtxid, | ||
| } => match (epicboxmsgid, epicboxtxid) { |
There was a problem hiding this comment.
TODO: do we need epicboxmsgid here still? Maybe for legacy compat (Made message)
|
Closed, solely for reference. Actual PR will come from a different, cleaned up branch. |
1 participant
This PR includes all of the changes made for Epicbox Protocol v3.1.0.
It should not be merged before a substantial review and cleanup (I will be doing this shortly).