search

dapphub / ds-token

A simple and sufficient ERC20 implementation
https://dapp.tools/dappsys/ds-token.html
GNU General Public License v3.0
224 stars 76 forks source link

Adding LogNote event #11

Closed gbalabasquer closed 7 years ago

rainbreak commented 7 years ago

For consistency with erc20 and also principle of least surprise, I'm thinking there should be Mint / Burn events as well.

gbalabasquer commented 7 years ago

I think is a good idea, but then, should we keep LogNote or we just leave the new events. I like the idea of note modifier but if it is just a duplication I'm not sure if makes that much sense.

nmushegian commented 7 years ago

@rainbreak how does the principle of least surprise apply when you turn to another ds-pkg - do you expect each one to have the simple Capitalized event-for-function convention like erc20? I was trying to distance ourselves from that. I guess tokens will always be extra special due to erc20, but who knows

rainbreak commented 7 years ago

@nmushegian no, I don't expect ds-pkg to follow the CapitalEvent per function format overall (although note may need some intro / usage documentation). I'm seeing DSToken as a special case because it is so tightly linked to erc20 idioms. I wouldn't extend this beyond Mint and Burn though (e.g. to auth), it's just that they reflect changes to the core token state.

It's a tough call because you ideally want ds-pkg to be consistent throughout, which is why I'd support adding note to everything as well.

nanexcool commented 7 years ago

@rainbreak only issue with adding note to everything is duplication of events (like now transfer and transferFrom send one LogNote and one Transfer) which is also the reason we didn't add to push and pull because it would send two LogNotes and one Transfer. But we're fighting against ERC20 here so I don't know what the right solution is.

dbrock commented 7 years ago

I think @rainbreak is right in almost every case but I have to disagree on this one :heart: