cmditch / elm-ethereum

dApps in Elm
https://package.elm-lang.org/packages/cmditch/elm-ethereum/1.0.1/
BSD 3-Clause "New" or "Revised" License
146 stars 21 forks source link

1.0 #33

Closed JasoonS closed 7 years ago

JasoonS commented 7 years ago

I fixed up the fromWei toWei functions. They work fully, via manual testing. I will setup unit testing soon just to make sure.

As a side note, please let me know how you want to structure your git. It seems strange to me that you deleted the dev-branch without merging it into main... Also keeping all of the smaller changes make it much easier too look back and find previous changes; it also makes it easier for others to follow the changes that have been made to the code.

cmditch commented 7 years ago

First, much thanks for contributing this!

Second, apologize for all the git mayhem that the repo has been subject to. My thoughts were, since I'm doing the vast majority of building this library during these beginning stages (and it's explicitly not been beta released yet), I wanted to keep the history rather clean. Once it's released, and others start using/contributing, I'll make sure much better gitting gets goin. 😛

I deleted the dev branch because I just merged test-suite (a dev fork) into master. I should have merged test-suite into dev and then into master. The sudden shift to having to work on web3 1.0 caused me some shock admittedly. Nothing had happened in my dev after I started working on test-suite so nothing was lost - I do understand you were working off dev, but you did what I'd hoped, and PR'd to 1.0, thanks.

Third, I've not much git w/ teams experience, so this has been an interesting learning process for me, and looking at what others do / best practices. I'm very open to discussion on this, and am glad you said something. Let's talk on how we should structure it:

I imagine we always have a dev branch and we just commit to that, then squash and commit dev to master when necessary. This way the master history is clean, but all the commit history is still maintained in dev? Let me know your thoughts. @ngmiller & @JasoonS

Lastly, before we merge this, let's move fromWei, toWei, and bigIntToWei into a Web3.Utils module to mirror what web3.js 1.0 is doing.

nickmonad commented 7 years ago

I imagine we always have a dev branch and we just commit to that, then squash and commit dev to master when necessary.

This is nice in that you can keep all commits in a dev branch, and only the squashed/trimmed commits in master, but poses some issues from a multi-dev standpoint. It's much, much easier to coordinate changes, understand what is happening, and prevent nasty merge conflicts if devs do not commit on each others branches, so I think a singular dev branch we all commit to isn't a great idea long-term.

If you're instead suggesting that this dev branch is blessed, such that development work is merged into dev (via a PR), which is then in turn, merged into master (via another PR), I'm on board with that.

As an anecdote, in my ~5 years of git experience - I have never run into a situation where knowing the exact string of commits leading to a change was necessary. I've needed to know who made a change (via git blame), but not all the tiny steps they made to get to that point. If you want to easily make reversions, remember that git reversions are never easy - you're almost always much better off treating the git history as additive, i.e. just make a new branch, fix the thing, and merge. If you want to understand the thought process behind a dev's work, git history is rarely a good way of doing that, unless that dev is particularly savvy to constantly squashing their local history to keep a clean record with actually useful commit messages; in which case, that argument just extends naturally into squashing all dev commits into one with a list of important changes (yes, commit messages can be more than one line long) prior to merge into master anyway.

TL;DR whichever branching strategy that doesn't involve us committing onto each others branches should work just fine.

cmditch commented 7 years ago

development work is merged into dev (via a PR), which is then in turn, merged into master (via another PR), I'm on board with that.

This sounds good to me.

nickmonad commented 7 years ago

@cmditch Sweet. This is actually a common pattern we've used at Workiva. We essentially treat the next minor (or sometimes major) version bump as a CI (continuous integration) branch that is prefixed as such: ci-1.0.0 for instance. This allows us to keep master absolutely pristine in the event of an emergency bug fix that must be made to master and released. And then, when that ci version is ready for release, that entire branch is PR'd against master with a list of all the tickets that went into it.

JasoonS commented 7 years ago

Cool guys :)

I have never run into a situation where knowing the exact string of commits leading to a change was necessary.

All I meant with the smaller commits comment was its really nice to be able to look at a commit heading and then look at the diff and see only changes related to that heading so it is easy to follow exactly what was fixed/changed. This is especially useful for projects like this where I may be away for a while and a quick look through the past diffs and quickly understand the changes (ie if a commit has changes to 2 features it often takes 2x longer to work out than 2 separate commits).

As for the actual git scheme to be used it's apples vs pears to me :+1:

nickmonad commented 7 years ago

@JasoonS That's reasonable. I see where you're coming from now. I need to get better about squashing in progress work and committing in better "units" myself.

JasoonS commented 7 years ago

I closed this because I thought it was already merged :see_no_evil: I'll move it to a Util file this evening (I'm in South Africa btw).

Btw, I'm curious about your guys motivation for starting this project, if you don't mind sharing :)

cmditch commented 7 years ago

Motivations? Good question.

We love Elm. We avoid javascript as much as we can manage. Frontend Ethereum development is monopolized by JS. We think in a world where small mistakes can be very expensive, we'd like the guarantees to be as strong as possible.


Background story:

This all started when I wanted to start working on a non-trivial DApp with lots of frontend state. After having worked in Elm quite a bit, I wasn't so keen on the idea of having to maintain a big React/Angular/Insert-JS-framwork-here codebase.

At first, I entertained the idea of using Elm w/ ports + Web3, but it became apparent this would require an unwieldy amount of ports/subs, along with an ugly mess of Msg's and decoders forced upon the user. The DX would have been atrocious.

Nick had the breakthrough idea of using Tasks and writing it in Elm native code. We heavily relied on Elm Http for guidance, as there is no documentation for writing Elm native code.

Once we got to web3 events/filters, we realized we needed something more than just Tasks. I came up with this insane hodgepodge of Native + Elm ports, where we called a port from within the native code. This required the user to define an elmShim for us, since that's the name we baked into the Elm native code, e.g.:

elm = Elm.Main.embed(element);
elmShim = elm;

It dawned on me, How does Elm's WebSockets library handle streaming? Thus, Elm's effect manager became an apparent option, and problem solved!

Now here we are. Having wrapped almost the entire soon-to-be deprecated web3.js codebase, I get to start the process back over again with web3.js 1.0 😄

JasoonS commented 7 years ago

Thats cool! Haha, I don't think it is soooo 'soon-to-be deprecated' but definitely soon enough!

Have you considered implementing much more of it directly in elm? (I know this would be much work... :see_no_evil:) But then theoretically the only only javascript you would need would be to manage the provider, no idea how doable this would be in practice... (from metamask)

part of the 1.0 is the acceptance that web3 is a good at being a library and not at being a standard -- so we should not inject web3 1.0

now we just need the ethereumProvider api finalized

Also the RPC interface is really not that challenging I feel, but it will be a ton of work never the less. There is actually a haskell-web3 implementation already.

But it is really nice the way 1.0 seems to be modularised! Definitely makes it easier/cleaner for us to work through the various components :)

nickmonad commented 7 years ago

haskell-web3 => 🎉

One major roadblock to a near-100% elm implementation is the crypto functionality, since there is no native binary type in elm, and sometimes, conversion to hex isn't an option prior to handling the signing and verification.

JasoonS commented 7 years ago

Ah, I see, elm is still young, one day elm will conquer the web+web3 ;) :grinning:

JasoonS commented 7 years ago

Updated :)

cmditch commented 7 years ago

I'll just go ahead and merge and make the minor changes mentioned above. Thanks @JasoonS !