Token Validator by expede · Pull Request #6 · Finhaven/EIPs

expede · 2018-02-23T20:31:06Z

The proposal that we've been working on. Should be ready for submission soon. Last round of feedback!

We've done a lot of discussion in person. It's all still fair game, from title to specifics.

…po (#5)

carchrae · 2018-02-23T23:20:03Z

EIPS/draft_token_validation.md

+    Created: 2018-02-14
+
+## Simple Summary
+A protocol for services providing financial transaction validation.


should include the word token in the summary.

carchrae · 2018-02-23T23:21:15Z

EIPS/draft_token_validation.md

+
+    EIP: <to be assigned>
+    Title: Token Validation
+    Author: Tom Carchrae<[email protected]>, Gleb Naumenko<[email protected]>, Brooklyn Zelenka<[email protected]>


the author order is backwards! i appreciate the alphabetical ordering, but you've done most of the work @expede

carchrae · 2018-02-23T23:22:53Z

EIPS/draft_token_validation.md

@@ -0,0 +1,181 @@
+## Preamble


not sure draft should be part of the filename

carchrae · 2018-02-23T23:41:52Z

EIPS/draft_token_validation.md

+interface TokenValidator {
+  function check(
+    address _token,
+    address _user


for consistency, rename to _address

🤔 Is _address too general, though? I get that it may represent an autonomous contract in addition to human users. Perhaps _subject or _toCheck? (ie: the one being checked?)

carchrae · 2018-02-23T23:42:15Z

EIPS/draft_token_validation.md

+
+> parameters
+> * `token`: the token under review
+> * `user`: the user to check


user -> address

carchrae · 2018-02-23T23:42:52Z

EIPS/draft_token_validation.md

+
+##### check/2
+
+`function check(address token, address user) public returns (uint8 resultCode)`


keep the args the same as the interface (eg, prefix vars with _)

carchrae · 2018-02-23T23:53:36Z

EIPS/draft_token_validation.md

+
+##### check/4
+
+`function check(address token, address from, address to, uint256 amount) public returns (uint8 resultCode)`


perhaps it would be nicer to give these each unique names. eg, checkTransfer and checkAddress

not dead set on this, just wondering if we could make them more specific/meaningful (but not too meaningful)

Yeah, fair point. It might be the Elixir/Erlang in me (which has a very similar name overloading system). It reads as implicit to me, but my background is very weird compared to most people 😛 check(address) and check(to, from, amount) seem pretty clear, but it also doesn't hurt to have some "duplication" (if it can even really be called that).

Thinking out loud, since all that the ABI knows about is the types, we really have:

check(address, address)

check(address, address, address, uint8)

Giving them longer/specific names would prevent namespace conflicts with like...

check(contractA, contractB) { return contractA == contractB; }

check(exchange, tokenA, tokenB, proposedConversionRate)

🤔

i like the elegance/terseness of check but i am not sure how it all plays with solidity.

it seems that you're saved from namespace inheritance clashes http://solidity.readthedocs.io/en/develop/contracts.html#overload-resolution-and-argument-matching

If there is not exactly one candidate, resolution fails.

but joy!

Return parameters are not taken into account for overload resolution.

Well, we both like check. We could toss it out to the community and see what they think. I'm not married to check(...) 🤷🏻‍♀️

carchrae · 2018-02-23T23:57:19Z

EIPS/draft_token_validation.md

+
+```solidity
+function approve(address spender, uint amount) public returns (bool success) {
+    if(validator.check(this, msg.sender, spender, amount) == !okStatusCode) {


not a fan of == !, you could make it != but even better, no negation just put L150 in the block if check(....) == okStatusCode

and use else to make the flow clear. return is an evil goto

return is an evil goto

I agree in this case. Are you not a fan of guard statements generally, or just here?

well, if truly a guard, wouldn't you use require() ?

but yeah, overall, not a big fan when, say, commenting out the return statement would cause the function to execute the else - just use else. more explicit, less bug chance, etc.

Well, I might not want it to blow up, but yes, indeed way more idiomatic in Solidity 👌🏻

carchrae · 2018-02-24T00:06:47Z

EIPS/draft_token_validation.md

+specify a transfer amount or recipient. This is intended for general checks,
+such as checking roles (admin, owner, &c), or if a user is on a simple whitelist.
+You may still use `check/4`, but passing dummy data just to satisfy a function contract
+is generally frowned upon.


sentence doesn't parse - i get what you are saying (you can use dummy values and just have one check) but it doesn't come through.

maybe better to keep the explanation direct and prescriptive rather than "hey you can do this but we say don't because it sucks" - we could have things like that in a different section (or later paragraph).

...or a blog post 😉

expede · 2018-02-24T01:00:08Z

@carchrae Thanks for the feedback. Fixed!

Once you get back from frolicking in the snow (and no earlier), gimme a ✅ if you're happy with the changes 🙆🏻‍♀️

carchrae · 2018-02-24T02:30:57Z

EIPS/token_validation.md

+
+> parameters
+> * `_token`: the token under review
+> * `_user`: the user to check


nit; _user isn't the same as _address above.

btw, i kind of agree with the comment you made (notified on slack, but not on github anymore) around _address being nebulous. perhaps _to would be a better descriptor? not certain on that though, but it makes it somewhat consistent with check/4 and i think it might make sense. but not sure.

I'm not totally certain about _to, since the arities are more than optional params. It could also be _from or something else. "Bob has surpassed his trade volume limit" or "Alice is a known criminal".

I think that this is still in line with Solidity, since even with the same arity we could have foo(uint8 _x), foo(uint8[] _xs) and foo(mapping(address => bool) _convenience). Of course as a best practice all foos should have some semantic similarity. An implementer would be able to rename a field to better suit their domain, too , since Solidity it only cares about the types. check(address _company, address _shareholder) { ... }.

carchrae · 2018-02-24T02:31:35Z

EIPS/token_validation.md

+This event MUST be fired on return from a call to a `TokenValidator.check/2`.
+
+> parameters
+> * `user`: the user to check


again, re nit above. should keep interface/events using same terms

@Arachnid

* init * permissions init * start of permissions service * Token Validator (#6) * File naming convention * Remove previous versions * remove reference to the old variable * Update frontmatter as per @Arachnid * Switch to byte

expede and others added 6 commits February 14, 2018 20:34

First pass

816711f

Spelling & clarification of security tokens; added link to R-Token re…

dfffd9e

…po (#5)

expand

5d192a9

Rename file

4a3710f

Spell check and rename file again

09913d2

Clarify status codes

9a93edd

expede self-assigned this Feb 23, 2018

expede requested review from bmann, carchrae, chiselinc and naumenkogs February 23, 2018 20:31

carchrae reviewed Feb 24, 2018

View reviewed changes

expede added 2 commits February 23, 2018 16:42

Addressing feedback

56bb3a1

Rename file and better if/else logic

b3bba27

typo

664a9dc

carchrae reviewed Feb 24, 2018

View reviewed changes

user -> subject

91114c6

carchrae approved these changes Feb 24, 2018

View reviewed changes

expede merged commit 31145e9 into master Feb 24, 2018

expede deleted the validator branch February 24, 2018 03:52

expede added a commit that referenced this pull request Feb 24, 2018

Token Validator (#6)

91720ed

expede added a commit that referenced this pull request Apr 3, 2018

Token Validator (#6)

9253e86

expede added a commit that referenced this pull request Apr 10, 2018

Token Validator (#6)

9426fe9


		##### check/2

		`function check(address token, address user) public returns (uint8 resultCode)`


		##### check/4

		`function check(address token, address from, address to, uint256 amount) public returns (uint8 resultCode)`

Conversation

expede commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

expede commented Feb 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

expede Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

expede commented Feb 23, 2018 •

edited

Loading

expede Feb 24, 2018 •

edited

Loading