Last unwraps

[mojoX911]

Resolves #253, #237

Removes unwraps from utill.rs and taker modules.

Note: setup_logger unwraps are kept intact. As this is only used once in cli apps, where unwraps at main.rs it doesn't have any effect. We will later refator this anyway for custom logger. Wasn't worth to add one more error variants just for this.

Added a few more unwraps removal from maker and market.

[Shourya742]

Ack. What are your thoughts on adding a NotFound error variant for all instances where we're currently using get with expect?

[mojoX911]

Ack. What are your thoughts on adding a NotFound error variant for all instances where we're currently using get with expect?

I am not entirely opposed. It feels equivalent to me. And more code to add variants just to denote a None, which is already a variant that we can match with an if let. So it's like writing let x = x;

If None needs to be handled we can already do that with if let matching.

Using expect basically says that "we never expect this to panic". And in rust we can somewhat claim such invariants if we architect the code correctly.

It can still panic, but that only means two things:
We screwed up somewhere in our logic/Something very nasty happened at your computer (your memory wiped out).

Both are valid cases for just panics.

expect is a very useful pattern to easily handle "unexpected" Nones. If you need more complex None handling, you can always use if let on it.

[KnowWhoami]

Ack few nits and small improvements, then it's good to go.

[KnowWhoami]

categroize this error in Wallet::Protocol -> we will get an error only when there's something wrong with our redeemscript which is directly related to coinswap protocol.

[mojoX911]

Damn, this opened up an entire can of worms. This is such a low-level util that changing its signature affects everything. But this needed to be handled or it can be quite problematic. A rouge maker can trigger this unwrap by sending non-segwit reedemscripts.

Stay tuned for another round of refactoring commits.

[mojoX911]

This also cannot be added to the wallet error as this function is used in the contract module, which is the core module. Adding this to the ProtocolError because that already encapsulates contract error so we can propagate easily.

[KnowWhoami]

This is such a low-level util that changing its signature affects everything. But this needed to be handled or it can be quite problematic.

yeah , this error needs to be propagated and should be properly handled by high level api's .

A rouge maker can trigger this unwrap by sending non-segwit reedemscripts.

Since this api is been used for both maker and taker components so the unwrap here can be a threat to both of them.
For eg: this api is used to get the scriptpubkey from mutliscript_redeemscript of funding tx in order to verify the proof of funding.
so for:

• Maker creates a seperate thread for handling the clients so having unwrap in this low level api will ultimately create unwrap inside the thread. So it will not cause much damage to it since Maker Server would still run but it would'nt be able to figure out the reason behind it and couldn't take any proper actions.

• Taker : Since it works in blocking mode -> thus having unwrap can panic the whole coinswap process and it wouldn't be able to figure out the exact reason behind this.

[KnowWhoami]

Stay tuned for another round of refactoring commits.

If it requires lot of refactorings -> then let's just have a followup PR for this issue otherwise it would create this PR a bit mess.

[mojoX911]

its gonna be a mess later too trying to fix it. Pushed the changes. lets bite the bullet here.

[KnowWhoami]

• coinswap/src/maker/config.rs

  Lines 249 to 252 in 972585c

  	fn write_default_maker_config(config_path: &PathBuf) {
  	let config = MakerConfig::default();
  	config.write_to_file(config_path).unwrap();
  	}

  We can make this code inline as already been for taker case and propagate the error.

• coinswap/src/wallet/funding.rs

  Line 280 in 972585c

  input_info.push(self.get_utxo((txid, vout)).unwrap().unwrap());

  Propagate the error.

[mojoX911]

We can make this code inline as already been for taker case and propagate the error.

Not sure what you mean, but only Taker didn't had propagation. Added that. Maker is fine.

Propagate the error.

There are many unwraps in funding module functions that are not being used. No point in spending time solving these unwraps. The only routine used currently is create_funding_txes_random_amounts which doesn't have unwraps, so we are fine. Commented the call sites of these functions. So they are disabled fully now.

[mojoX911]

There are weird overlaps between ContractError and ProtocolError. Life will be much simpler if these two are just merged into one single error. Doing that.

[mojoX911]

Addressed comments and other refactors from 7de0fba

[codecov]

Codecov Report

Attention: Patch coverage is 66.01307% with 52 lines in your changes missing coverage. Please review.

Project coverage is 74.83%. Comparing base (e2ee88f) to head (64fd3ec).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
src/protocol/contract.rs	70.00%	9 Missing ⚠️
src/taker/api.rs	71.42%	8 Missing ⚠️
src/wallet/swapcoin.rs	66.66%	6 Missing ⚠️
src/taker/error.rs	0.00%	5 Missing ⚠️
src/wallet/api.rs	44.44%	5 Missing ⚠️
src/wallet/funding.rs	0.00%	5 Missing ⚠️
src/taker/offers.rs	66.66%	4 Missing ⚠️
src/maker/error.rs	0.00%	2 Missing ⚠️
src/protocol/error.rs	0.00%	2 Missing ⚠️
src/wallet/error.rs	50.00%	2 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
- Coverage   78.01%   74.83%   -3.18%     
==========================================
  Files          32       32              
  Lines        4835     3998     -837     
==========================================
- Hits         3772     2992     -780     
+ Misses       1063     1006      -57     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[KnowWhoami]

There are weird overlaps between ContractError and ProtocolError. Life will be much simpler if these two are just merged into one single error. Doing that.

Yeah ,that will make things a bit simpler.
But still IMO, we should have that because right now, all errors are at same level under ProtocolError enum -> some of which are related to Contract ones -> so having a ContractError enum for those errors would make our error enum more modular.
what's your say?

[mojoX911]

Contracts are part of the protocol. I don't see any reason why we need a separate error for just contracts. And if we are going by definition, then that should only include script errors, and nothing else. That's what a "contract" is. Its redundant nomenclature. Better to get rid of it.

[Shourya742]

ACK