implement erc20 reducer, transfers #35

nionis · 2018-07-24T18:35:49Z

This implementation is only for transfers, we need to discuss on what is the best way to handle mints / burns / totalSupply

shrugs

fantastic start! added some nitpicks

shrugs · 2018-07-24T22:47:40Z

packages/gnarly-reducer-erc20/package.json

+    "type": "git",
+    "url": "[email protected]:XLNT/gnarly.git"
+  },
+  "author": "Matt Condon <[email protected]>",


please add yourself here!

shrugs · 2018-07-24T22:49:00Z

packages/gnarly-reducer-erc20/src/models/sequelize.ts

+    'erc20_balances',
+    {
+      id: { type: DataTypes.STRING, primaryKey: true },
+      darAddress: { type: DataTypes.STRING },


let's replace this with tokenAddress or similar; a DAR is a NFT-specific thing (distinct asset registry or whatever it was supposed to mean).

shrugs · 2018-07-24T22:51:24Z

packages/gnarly-reducer-erc20/src/models/sequelize.ts

+      id: { type: DataTypes.STRING, primaryKey: true },
+      darAddress: { type: DataTypes.STRING },
+      owner: { type: DataTypes.STRING },
+      balance: { type: DataTypes.STRING },


It may be helpful to have an unsafeBalance column that's a DECIMAL of some arbitrary precision. It's not ideal, since storing 256 bit numbers is not something postgres is designed to do, but I know a lot of people will want to sort by balance and otherwise do numeric queries, so it'd be good to support that at some basic level. You can see something similar in unsafeNumber from the block metadata reducer.

do you know what data type would be the best?

do we actually need it to be a string at all?

well, turns out the DECIMAL type can handle the 78 decimal points required by a 256 bit number as DECIMAL(78, 0) so perhaps it may not be necessary. google was not very helpfulf or best practices. I've asked twitter and will report back lol https://twitter.com/mattgcondon/status/1022645438229307392

@nionis nobody responded on twitter, but looking at various docs, it seems like a decimal is fine. So let's make the primary datatype a DECIMAL(76, 0) and a backup strBalance or balanceStr (your preference) that holds the raw data just in case

shrugs · 2018-07-24T22:53:19Z

packages/gnarly-reducer-erc20/src/reducer.ts

+      debug('transferring %d tokens from %s to %s', value, from, to)
+
+      // turn value into BN value
+      value = new BN(value)


let's use an immutable pattern here to avoid any confusion about the types

transfer: (from: string, to: string, rawTokenValue: string) => { const tokenValue = new BN(rawTokenValue) ...

shrugs · 2018-07-24T22:57:13Z

For handing mint/burn mechanics, lets implement the minimum viable concept of

If Transfer to 0x0 or Mint, add the value to the addr and emit a ERC20_MINT reason.
If Transfer from 0x0 or Burn, subtract the value from the addr and emit an ERC20_BURN reason.

That should get us 90% of compatibility, I think. Let's use the OpenZeppelin Burn and Mint event abis.

nionis · 2018-07-25T15:12:32Z

@shrugs should we make 0x0 balance to remain 0 when there its Transfer from 0x0 to X address ?

shrugs · 2018-07-27T00:53:37Z

@nionis It seems like an arbitrary thing, but I do prefer the idea of keeping burns literal. That way we should be able to get totalSuppy by doing something like SELECT SUM(balance) as totalSupply where address = '0xabcd'.

nionis · 2018-08-03T18:32:37Z

@shrugs Mint / Burn always emit Transfer function, so there is no need to check for those events unless we want to specifically store the MINT_REASON or BURN_REASON

so I would just look for Transfer event, and then check from / to values, if they are 0x0 then I do MINT_REASON / BURN_REASON

sounds good?

shrugs · 2018-08-03T18:34:41Z

Yup! There might be edge cases with some tokens that don't emit the correct transfer event, but I expect it's not something we have to worry about yet. Let's definitely note that the reason being different between transfers mints and burns; that's something i expect people would like to query directly, instead of inferring it from the fact that the _to is 0 or vise versa.

…

-matt

On Fri, Aug 3, 2018, 11:32 Steve ***@***.***> wrote: @shrugs <https://github.com/shrugs> Mint / Burn always emit Transfer function, so there is no need to check for those events unless we want to specifically store the MINT_REASON or BURN_REASON so I would just look for Transfer event, and then check from / to values, if they are 0x0 then I do MINT_REASON / BURN_REASON sounds good? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#35 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsGYsgSqngW47mK5Y8VSXHQk0DImBfks5uNJdFgaJpZM4VdQEB> .

… reasons, and some housework

nionis · 2018-08-03T18:57:33Z

packages/gnarly-reducer-erc20/src/reducer.ts

+const MINT_REASON = 'ERC20_MINT'
+const BURN_REASON = 'ERC20_BURN'
+
+const isZeroAddress = (address: string) => address === '0x0000000000000000000000000000000000000000'


probably there is a better way to do this, ideas?

I expected there would be an isNullAddress utility in web3, but apparently there isn't. Let's leave this here for now, but I've added issue #39 to track it

nionis · 2018-08-03T18:58:34Z

packages/gnarly-bin/src/index.ts

+    Sequelize,
+    sequelize,
+  ))(
+    ZRX_ADDRESS,


I hope using 0x token in bin is not an issue, I used that one because it has alot of traffic and its nice for testing

shrugs · 2018-08-04T00:26:48Z

packages/gnarly-reducer-erc20/src/reducer.ts

+
+  const makeActions = (state: IERC20Tracker, { operation, emit }) => ({
+    transfer: (from: string, to: string, rawTokenValue: string) => {
+      debug('transferring %d tokens from %s to %s', rawTokenValue, from, to)


nit: let's make this

debug('transferring %d %s tokens from %s to %s', rawTokenValue, key, from, to)

so the logs mention which reducer is being run

shrugs · 2018-08-04T00:27:18Z

packages/gnarly-reducer-erc20/src/reducer.ts

+        })
+      }
+
+      operation(() => {


nit: this doesn't actually need to be within an operation; it's implied by the because block that this is executed in.

I see! functions like because / operation / emit etc, we could make some documentation on them cause I am not sure how they work tbh

you're totally right 😁

I'm not sure if it's worth investing in a docs wiki or anything just yet, so I'll add an issue to append this information to the README

implement erc20 reducer, transfers

42c122a

shrugs reviewed Jul 24, 2018

View reviewed changes

erc20 reducer: updated balance to DECIMAL type, created MINT and BURN…

27a76d7

… reasons, and some housework

nionis commented Aug 3, 2018

View reviewed changes

shrugs reviewed Aug 4, 2018

View reviewed changes

erc20 reducer: improve debug and housework

4500167

shrugs merged commit 1c8db1d into XLNT:master Aug 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement erc20 reducer, transfers #35

implement erc20 reducer, transfers #35

nionis commented Jul 24, 2018

shrugs left a comment

shrugs Jul 24, 2018

shrugs Jul 24, 2018

shrugs Jul 24, 2018

nionis Jul 25, 2018

shrugs Jul 27, 2018

shrugs Jul 31, 2018

shrugs Jul 24, 2018

shrugs commented Jul 24, 2018

nionis commented Jul 25, 2018

shrugs commented Jul 27, 2018

nionis commented Aug 3, 2018

shrugs commented Aug 3, 2018 via email

nionis Aug 3, 2018

shrugs Aug 3, 2018

nionis Aug 3, 2018

shrugs Aug 4, 2018

shrugs Aug 4, 2018

nionis Aug 4, 2018

shrugs Aug 4, 2018

implement erc20 reducer, transfers #35

implement erc20 reducer, transfers #35

Conversation

nionis commented Jul 24, 2018

shrugs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrugs commented Jul 24, 2018

nionis commented Jul 25, 2018

shrugs commented Jul 27, 2018

nionis commented Aug 3, 2018

shrugs commented Aug 3, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment