kangarang / tcr-ui

A client-side shell to interact with token-curated registries
MIT License
68 stars 28 forks source link

Wrong minDeposit calculated #106

Closed d-xo closed 6 years ago

d-xo commented 6 years ago

Hey 👋

So when I use my own deployed TCR (I tried with v1.0 and v1.1.0 from https://github.com/skmgoldin/tcr), the minDeposit is always showing up as 0, which makes interacting with the TCR impossible as all applications fail (assuming a non-zero min deposit). The tokens were generated new for each TCR by the TCR deployment scripts.

I tracked the issue down to this line: https://github.com/kangarang/tcr-ui/blob/master/src/modules/home/sagas/contracts.js#L186

When I change it from minDeposit: baseToConvertedUnit(minDeposit['0'].toString(10), tokenDecimals), to minDeposit: minDeposit['0'].toString(10), then the minDeposit is calculated correctly.

The unit conversion code is working fine for the other TCR contracts, is there some deployment step that I'm missing?

If you want to reproduce the issue, you can add the following as a TCR:

{
    name: 'Humans',
    registryAddress: '0xe05d7224648a81798f3e0665ce736eac31703ccf',
    votingAddress: '0xe05d7224648a81798f3e0665ce736eac31703ccf',
    tokenAddress: '0xe05d7224648a81798f3e0665ce736eac31703ccf',
    tokenSymbol: 'HMN',
    tokenName: 'Human',
    multihash: 'Qma3uaQ4RRSR1dR4vNj7CzFAHG5aY42tgBoW9HPWv89msT',
}

p.s. Really appreciate the work on this repo, I've been using it to play around with and understand TCR's and it's really a super valuable resource. 🙏 🙏

kangarang commented 6 years ago

@xwvvvvwx hi! glad it's been useful for you, and thanks for digging into the codebase

yes, others and myself included have been thrown off by the minDeposit. what's happening is that the deployment script uses a config file to set various initial parameters of the tcr system, and the config file does not account for the token's decimals, the value indicating the number of digits one should use when making unit conversions. typically 18, like ETH (more info)

we just merged this fix into the develop branch, and hopefully we will have some time next week to merge it into master. til then, adding 18 0's to both the minDeposit and the pMinDeposit should do the trick for ya

d-xo commented 6 years ago

Ah!

That makes sense now. Thanks so much 🙏 🙏