robinmonjo / coincoin

Blockchain based cryptocurrency proof-of-concept built with Elixir. Feedback welcome
402 stars 54 forks source link

Basic token implementation #7

Closed robinmonjo closed 7 years ago

robinmonjo commented 7 years ago

Token system implementation. This PR includes:

This is mostly complete, even if some tests are missing. TODOs before merging:

yordis commented 7 years ago

@robinmonjo do you really need that credo file? I would push for the default credo config until it fail. Specially working in the OSS ecosystem IMO

yordis commented 7 years ago

change repository name to coinstack

What is the future of this repo. I am interested on what you are doing 😄

robinmonjo commented 7 years ago

@yordis thank you very much for your review 😊. I want to change the name of the repo because it was originally "blockchain" but with this PR it's more than just a blockchain. So I want to rename it "coinstack".

As for the future of this repo, I haven't much more ambitions. For me it was really a way to work with Elixir and to learn how cryptocurrencies work. So I will probably polish it a bit (add typespecs for example), increase test coverage and work on the blockchain_web to create a UI.

How would you see this project evolve ?

PS: for the credo file, I just changed the "line length" check. I really don't like that linters today still use the 80 characters limit.

I'll take care of your comments before merging. Again thank you !

yordis commented 7 years ago

PR it's more than just a blockchain.

I would say that I a sign that you will need to actually split into more Domains 😉 Separation of concern, leave the blockchain piece as it's right now and do something on top of it. Just create some namespace and don't think twice on creating more OTPs, right now I can't see what are you heading but I could help with some architecture decisions, I am also really new into cryptocurrency so we learn togther

About credo, just add that rule and done. I prefer to force a little bit myself and have consistency with other repos.

robinmonjo commented 7 years ago

So I deleted the credo file, plus, if used without --strict credo doesn't report line length warning, so I'll live with that.

yordis commented 7 years ago

@robinmonjo I would suggest to put all the code behind CoinStack namespace, like CoinStack.Token and so on

yordis commented 7 years ago

@robinmonjo I notice https://github.com/robinmonjo/blockchain/blob/c9f691288696a9cb32d18a6b33698df35259a152/apps/blockchain/lib/blockchain/block.ex#L38 what would happen of Proof of Stake?

Or rather Proof of X I think is bette to actually create a module that receive the block and try to do X computation so basically we can swap whatever Proof of X we want and we need to just swap the configuration of the blockchain

robinmonjo commented 7 years ago

actually the Blockchain.Block.compute_hash/1 function is just a function that compute the sha256 hash of the block, that ends up in the hash field. It is not directly related to Proof-of-Work.

Has for what it would take to switch to another method for proof of something, I don't really know because Proof-of-Stake and Proof-of-work are really two different things and I can't imagine an API they could share.

yordis commented 7 years ago

@robinmonjo shit sorry wrong line https://github.com/robinmonjo/blockchain/blob/c9f691288696a9cb32d18a6b33698df35259a152/apps/blockchain/lib/blockchain/block.ex#L47

But the point is to move any computation or logic to their own module so we do not have all over the place or more than one implementation in the same module.

yordis commented 7 years ago

Those three

 # https://en.bitcoin.it/wiki/Proof_of_work
  def perform_proof_of_work(%Block{} = b) do
    {hash, nounce} = proof_of_work(b)
    %{b | hash: hash, nounce: nounce}
  end

  defp proof_of_work(%Block{} = block, nounce \\ 0) do
    b = %{block | nounce: nounce}
    hash = compute_hash(b)
    case verify_proof_of_work(hash) do
      true -> {hash, nounce}
      _ -> proof_of_work(block, nounce + 1)
    end
  end

  def verify_proof_of_work(hash) do
    difficulty = Application.fetch_env!(:blockchain, :pow_difficulty)
    prefix = Enum.reduce 1..difficulty, "", fn(_, acc) -> "0#{acc}" end
    String.starts_with?(hash, prefix)
  end

move it out to some module call ProofOrWork or whatever will be and that module implements some protocol probably

robinmonjo commented 7 years ago

@yordis found some time tonight, what I did:

I haven't scoped everything under the Coinstack domain. We don't need it for now.

I'm pretty happy with the state of this PR, you made really interesting feedbacks ! I think I'm gonna go over the READMEs one more times and merge it. What do you think ?

Once merged, if you go over the code and find things that could be improved open an issue, I would be happy to handle it !

yordis commented 7 years ago

if it's working then go and merge to master so we do not keep looking at big commits to review 😄 I am more than happy to keep giving you feedback. Sometimes just asking you questions or debating about the implementation we both learn.

So far so good 💟

P.S: @robinmonjo did you read my latest comments? Because I think I didn't hit the comment button and I am not sure now. You have to probably expand the comments

robinmonjo commented 7 years ago

@yordis, again, thank you for your review !! I'm doing Elixir on my own and that's really interesting to have feedbacks from a more experienced Elixir dev. I think we can merge this PR, but please, keep "hitting" me with feedbacks that's the best way for me to improve 😊.

Will go over everything one last time tomorrow and merge.