citadel-tech / coinswap

Functioning, minimal-viable binaries and libraries to perform a trustless, p2p Maxwell-Belcher Coinswap Protocol
https://gist.github.com/chris-belcher/9144bd57a91c194e332fb5ca371d0964
Other
73 stars 46 forks source link

Last unwraps #290

Open mojoX911 opened 2 weeks ago

mojoX911 commented 2 weeks ago

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.

mojoX911 commented 1 week ago

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 commented 6 days ago
mojoX911 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

Addressed comments and other refactors from https://github.com/citadel-tech/coinswap/pull/290/commits/7de0fbae5f1e090e147a0d92ffdc177be2ebd3b3

codecov[bot] commented 1 day ago

Codecov Report

Attention: Patch coverage is 74.57627% with 45 lines in your changes missing coverage. Please review.

Project coverage is 79.12%. Comparing base (dff5c90) to head (bbb56b0). Report is 20 commits behind head on master.

Files with missing lines Patch % Lines
src/protocol/contract.rs 70.00% 9 Missing :warning:
src/taker/error.rs 0.00% 5 Missing :warning:
src/wallet/api.rs 50.00% 5 Missing :warning:
src/wallet/funding.rs 0.00% 5 Missing :warning:
src/taker/offers.rs 66.66% 4 Missing :warning:
src/wallet/swapcoin.rs 80.95% 4 Missing :warning:
src/taker/api.rs 93.47% 3 Missing :warning:
src/maker/error.rs 0.00% 2 Missing :warning:
src/protocol/error.rs 0.00% 2 Missing :warning:
src/taker/routines.rs 33.33% 2 Missing :warning:
... and 3 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #290 +/- ## ========================================== + Coverage 78.92% 79.12% +0.20% ========================================== Files 32 32 Lines 4834 4795 -39 ========================================== - Hits 3815 3794 -21 + Misses 1019 1001 -18 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features: