Skip to content
This repository has been archived by the owner on Jan 1, 2020. It is now read-only.

ESC proposal #8

Merged
merged 31 commits into from
May 5, 2018
Merged

ESC proposal #8

merged 31 commits into from
May 5, 2018

Conversation

expede
Copy link

@expede expede commented Apr 30, 2018

Leading up to submitting to Ethereum/EIPs

Why created: 2018-05-04, you ask?

...which is a bit silly. If it's ready before then, so be it. It's mostly fun because our last one happened to land on Feb 14, so we're continuing the trend of easy to remember dates 😉

@expede expede self-assigned this Apr 30, 2018
@expede expede requested review from carchrae and naumenkogs April 30, 2018 05:34
@expede
Copy link
Author

expede commented Apr 30, 2018

@danmerino would love your thoughts as well 😄 Any missing categories, confusing stuff, &c

@expede
Copy link
Author

expede commented Apr 30, 2018

Submit to Ethereum/EIPs

@expede expede requested a review from avivash April 30, 2018 17:15
@danmerino
Copy link

Looks good to me. I could not find anything I could not do with the current set and because its extensible it should fit most scenarios! One thing that would be great would be to see some use of control structures in JS to be able to handle cases cleanly. Something like this in elixir:

case Repo.insert(changeset) do
  {:ok, user} ->
    case Guardian.encode_and_sign(user, :token, claims) do
      {:ok, token, full_claims} ->
        important_stuff(token, full_claims)

      error ->
        error
    end

  error ->
    error
end

@expede
Copy link
Author

expede commented May 1, 2018

Good point, @danmerino ! I'll add that to the support lib 😄

@carchrae
Copy link

carchrae commented May 1, 2018

on your theme: https://www.youtube.com/watch?v=U9t-slLl30E

in terms of comments;

@expede
Copy link
Author

expede commented May 4, 2018

@carchrae

perhaps add a short example to the abstract to make it super clear what the purpose is. just a two line example, showing a simple context, and perhaps the now vs proposal situation.

✅ Done

in solidity you have toCode but in js you have fromCode and fromString - (i think js fromString should be toCode)

Yep, will do. Different repo (not directly part of the EIP). I've opened an issue on that repo. UPDATE: this has a WIP PR

the examples are great, but i wonder if there isn't a more elegant way to map a validator to the hex for app specific reasons, (eg, x05 in https://github.com/Finhaven/EthereumStatusCodes/blob/2679e92250462cad6a34af863d3ec66c732c203a/contracts/validators/FinancialValidator.sol#L7 ). like if there was a helper that mapped `FinancialValidator.reasons = customCodes([ 'insufficientFunds']) - or some other method to avoid pointing to magic strings. )

Hmm, yeah.

That example is actually a bit broken. 05 currently has no meaning, and is not flagged as an application-specific code. UPDATE: The range (above/below) here is probably a common use case. Added these codes to the proposal 👍🏻

For mapping, I've merged the PR open on that repo (PR 14)

@expede
Copy link
Author

expede commented May 5, 2018

in short, i think this work is complimentary rather than similar

💯 Will add

Also

  • Revert with reason also lacks translation
  • Once this is more mature, we can do an EIP to use these in transaction.status

@expede
Copy link
Author

expede commented May 5, 2018

@carchrae Is this more along the lines of your suggestion?

https://github.com/Finhaven/EIPs/blob/ecf19dff4a08257bfea7ddd8333f858d6067ad5e/EIPS/eip-status-codes.md#abstract

It feels a bit awkward to me. I've been trying to keep it under 200 words (as per the EIP submission guidelines), so it feels very broad and a bit repeat-y. But then again, maybe I've been staring at it too long 😛

Thoughts?

@carchrae
Copy link

carchrae commented May 5, 2018

yes, too long. two sentences! :)

also, you may want to be more specific than 'language' - this is a framework to define common return codes, which is also extensible to allow application specific return code.

@carchrae
Copy link

carchrae commented May 5, 2018

this bit works well in abstract

Much like HTTP codes, having a standard set of known codes has many benefits for developers. They remove friction from needing to develop your own schemes for every contract, makes inter-contract automation easier, and makes it easier to broadly understand which of the finite states your request produced. Importantly, it makes it much easier to distinguish between expected errors states, and truly exceptional conditions that require halting execution.

@carchrae
Copy link

carchrae commented May 5, 2018

i think the revert discussion might be more useful in the details below. it is super important, but not the main thrust.

@expede
Copy link
Author

expede commented May 5, 2018

@carchrae 🙏🏻 thanks for helpful feedback!

@expede expede merged commit e0570ac into master May 5, 2018
@expede expede deleted the esc branch May 5, 2018 20:42
expede added a commit that referenced this pull request May 5, 2018
expede added a commit that referenced this pull request May 5, 2018
expede added a commit that referenced this pull request May 8, 2018
expede added a commit that referenced this pull request May 9, 2018
expede added a commit that referenced this pull request May 9, 2018
expede added a commit that referenced this pull request May 9, 2018
expede added a commit that referenced this pull request May 9, 2018
expede added a commit that referenced this pull request May 15, 2018
expede added a commit that referenced this pull request May 15, 2018
expede added a commit that referenced this pull request May 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants