-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basic token implementation #7
Conversation
Those three
move it out to some module call |
String.starts_with?(hash, prefix) | ||
end | ||
|
||
defp difficulty, do: Application.fetch_env!(:blockchain, :pow_difficulty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that pow_difficulty
is actually related to ProofOfWork
which I think it is no?!, then do
config :blockchain, Blockchain.ProofOfWork,
pow_difficulty: 10_000
Application.fetch_env!(:blockchain, Blockchain.ProofOfWork, :pow_difficulty)
Dont over use the :blockchain
scope, this way you will help people to understand what is going on with that config.
@@ -73,7 +73,7 @@ defmodule Blockchain.Mining do | |||
end | |||
|
|||
defp mine_block(%Block{} = b) do | |||
mined_block = Block.perform_proof_of_work(b) | |||
mined_block = ProofOfWork.compute(b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be better to make this a config that you could swap? If not then this implementation force
you to use PoW which it is fine but in my head is probably better to create some config, even we can plug more than one computation (but that's too fancy for now)
@yordis found some time tonight, what I did:
I haven't scoped everything under the 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 ! |
apps/blockchain/config/config.exs
Outdated
@@ -31,4 +31,7 @@ use Mix.Config | |||
|
|||
config :blockchain, | |||
port: String.to_integer(System.get_env("P2P_PORT") || "5000"), | |||
proof_of_work: if(Mix.env == :test, do: Blockchain.Test.ProofOfWork, else: Blockchain.ProofOfWork) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do this
if you comment out this line
# import_config "#{Mix.env}.exs"
then you need to create the respected file for the environment. Because it will load that file to
What you could do is to put the default configuration on config/config.exs
and then use config/test.exs
for override the values.
Side note, the override is using a deep overwriting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok took care of that
@@ -14,15 +14,15 @@ defmodule Blockchain.ChainTest do | |||
b = | |||
"some data" | |||
|> Block.generate_next_block() | |||
|> ProofOfWork.compute() | |||
|> proof_of_work().compute() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have question because you are 100% more involve than me.
What would happen if we want to support something like PoW + PoT together, IF I am not mistaking Dash coin does that, it split it up into 50% 50% work.
Also, let say tomorrow I want to create ProofOfSharing (PoSH) how would that look like?
When I told you to move it into some module I was actually thinking on create some config with the module (not called proof_of_work
because defeat the idea 😄 ) where we can setup either a list or just a module and you will call the compute
function on it, which is why I told you about the protocol.
So that way you can have as many implementations of how you reward or do any computation of this kind.
Something like
config :blockchain,
some_better_name_key: [ProofOfWork, ProofOfStake, ProofOfShares]
# or
config :blockchain,
some_better_name_key: [
pow: ProofOfWork, # <- for the sack of the demo whatever key value pair
pos: ProofOfStake,
posh: ProofOfShares
]
the last example could be useful when you want to control that you HAVE to use specific module for X reason. I would even stick with the array for now 😄 we need study what other coins do differently and more important where the crypto world is moving to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what's your point here. However for now I only know proof-of-work, I would have to dig more into other protocols that replace PoW to have a better idea on how this could be implemented and how we could go to what you suggest here. If that's ok with you, I'll move your really good comment to an issue and spend times on it later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue here: #8
@@ -105,5 +106,6 @@ defmodule Blockchain.P2P.Command do | |||
|> Payload.mining_request() | |||
|> Payload.encode!() | |||
|> Server.broadcast() | |||
Mining.mine(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|> Mining.mine()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed bad, name here, I changed it to MiningPool.add
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 |
@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. |
Token system implementation. This PR includes:
This is mostly complete, even if some tests are missing. TODOs before merging:
coinstack