Refactoring send.rs by introducing transfer trait

[twosatsmaxi]

Motivation

Catching bugs early for example:

• If transfer trait has a fixed contract, bugs like this could be cought early by making sure that the postage is used for new transfer implementation - A useless postage was added to the sending runes tx? #3620

Code Quality

• More extensibility, for example, the most requested features in ordicord like splitting rune or send to many can have its own transfer trait implementation without bloating send.rs file.
• Cleaner code, for example:
  • sending_inscription boolean can be moved out of the create_unsigned_send_satpoint_transaction method and to its trait implementation (not a big deal but just a bit cleaner code)
  • send.rs is now just 52 lines of code after the refactor and super readable ❤️

[onchainguy-btc]

Maybe we can merge #3437 before? 👀 It allows for different features to be built on top like #3025 or #3640

[twosatsmaxi]

@raphjaph @casey this ^ is ready for review now, no functionality changes, just refactoring.

[casey]

I personally don't think this is a huge increase in readability, since there's just more indirection in figuring out what's going on.

[twosatsmaxi]

I personally don't think this is a huge increase in readability.

But do you think though that it's a bit more readable?

[twosatsmaxi]

since there's just more indirection in figuring out what's going on.

It simplify things a bit for new folks who want to read about what's going on in send.rs.

@casey Some more Context on why I went ahead with this refactor: I wanted to implement the Even Split command for rune where I can split 10k of runes into 100 each, send.rs was a big and going through that code with different branches was lots of to-and-fro. If it had been like this, understanding what's going through would be much less time consuming for new contributors and can start checking in more features like split command quickly.

[twosatsmaxi]

I feel that even if it adds a bit more readability and simplicity, there is no harm in checking in to main branch since all unit tests are passing, unless it adds more complexity, which I don't think so.

[twosatsmaxi]

We were a way past "Rule Of Three" and the refactoring was a due in my opinion. Currently, there are 5 sends.

[casey]

To be clear: I closed this PR because I think it's a net negative and makes the code harder to read.

The rule of three is about deduplication. The PR is net +107 lines of code, and replaces a match statement which calls three different functions, with a trait with five different implementations. It is not deduplicating anything.

I think it's much worse and more complicated.

[casey]

• If transfer trait has a fixed contract, bugs like this could be cought early by making sure that the postage is used for new transfer implementation - A useless postage was added to the sending runes tx? #3620

This bug was fixed with a change to create_unsigned_send_runes_transaction. In this PR, this function was just moved from one place to another. I don't see that would have helped find that bug.

• More extensibility, for example, the most requested features in ordicord like splitting rune or send to many can have its own transfer trait implementation without bloating send.rs file.

Having six functions in a file is fine.

• send.rs is now just 52 lines of code after the refactor and super readable ❤️

It's super readable, but reading it doesn't tell you anything about what it does.

[twosatsmaxi]

The PR is net +107 lines of code.

but does # of lines code really matter in such cases? # of lines of code would increases if a piece of code is moved to different files with new imports, new functions definitions, empty lines tends to add to net # of loc.

In this PR, this function was just moved from one place to another.

The function wasn't moved to another place as is, only just implementation was moved. The function method signature was changed to adhere to method contract create_unsigned_transaction in the transfer trait.

This bug (the postage not being used for send runes) was fixed with a change to create_unsigned_send_runes_transaction. In this PR, this function was just moved from one place to another. I don't see that would have helped find that bug.

It's true that It wouldn't have helped finding that bug, but while writing the new "send rune" implementation itself, it would have enforced the author to use postage (due to trait contract for transer being there) In the absense of the transfer contract, there is no way to enforce/nudge that postage need to be used. The bug was later fixed by adding postage to create_unsigned_send_runes_transaction function here