dio-lab-open-source icon indicating copy to clipboard operation
dio-lab-open-source copied to clipboard

feat: add michelmouraresende profile

Open michelmouraresende opened this issue 9 months ago • 2 comments

Descrição

Adiciona o profile README de michelmouraresende

Tipo de alteração

  • [x] Resolução do Desafio Profile README (envie APENAS o arquivo community/SEU_USERNAME.md)
  • [ ] Correção de bug
  • [ ] Nova funcionalidade
  • [ ] Alteração na documentação
  • [ ] Outro (especifique)

Checklist

  • [x] Minhas alterações não deletam partes do projeto
  • [x] Minhas alterações não introduzem novos problemas
  • [x] Minha contribuição está de acordo com o Guia de Contribuição

Comentários adicionais

N/A

michelmouraresende avatar May 05 '24 19:05 michelmouraresende

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

onchainguy-btc avatar May 06 '24 13:05 onchainguy-btc

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

twosatsmaxi avatar May 08 '24 04:05 twosatsmaxi

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.

casey avatar May 14 '24 04:05 casey

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 avatar May 14 '24 06:05 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.

image

twosatsmaxi avatar May 14 '24 06:05 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 avatar May 14 '24 06:05 twosatsmaxi

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

image

twosatsmaxi avatar May 14 '24 06:05 twosatsmaxi

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 avatar May 14 '24 07:05 casey

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.

casey avatar May 14 '24 07:05 casey

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

twosatsmaxi avatar May 14 '24 08:05 twosatsmaxi