WIP: Contribution contracts tests #159

Closed
haythem96 wants to merge 20 commits from tests/contracts-contribution into master
Showing only changes of commit 6b0083eeb8 - Show all commits

View File

@ -1,4 +1,3 @@
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
const ethers = require('ethers');
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
const namehash = require('ethers').utils.namehash;
// eslint-disable-next-line no-undef
@ -48,7 +47,6 @@ const mineBlock = function() {
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
contract('Contribution app', (accounts) => {
// eslint-disable-next-line no-undef
let ethProvider = new ethers.providers.Web3Provider(web3.currentProvider);
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
let kernelBase, aclBase, daoFactory, r, dao, acl, contribution, token, contributor;
const root = accounts[0];
@ -234,8 +232,6 @@ contract('Contribution app', (accounts) => {
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
it("veto contribution", async () => {
const contributionId = await contribution.contributionsCount();
let contributionObject = await contribution.getContribution(contributionId.toNumber());
console.log("veto block: " + contributionObject[7]);
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
console.log("current block: " + await ethProvider.getBlockNumber());
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
await contribution.veto(contributionId.toNumber(), {from: root});
// eslint-disable-next-line no-undef
assert(contributionObject[9], true);
@ -282,8 +278,6 @@ contract('Contribution app', (accounts) => {
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
await mineBlock();
let contributionObject = await contribution.getContribution(contributionId.toNumber());
console.log("claim block: " + contributionObject[7]);
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
console.log("current block: " + await ethProvider.getBlockNumber());
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
await contribution.claim(contributionId);
// eslint-disable-next-line no-undef

haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
haythem96 commented 2019-08-02 16:29:19 +00:00 (Migrated from github.com)
Review

this one revert for some reasons

this one revert for some reasons
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:39:17 +00:00 (Migrated from github.com)
Review

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:09 +00:00 (Migrated from github.com)
Review

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
raucao commented 2019-08-02 16:40:45 +00:00 (Migrated from github.com)
Review

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:24:35 +00:00 (Migrated from github.com)
Review

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
haythem96 commented 2019-08-03 11:37:47 +00:00 (Migrated from github.com)
Review

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:58:16 +00:00 (Migrated from github.com)
Review

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
raucao commented 2019-08-03 11:59:37 +00:00 (Migrated from github.com)
Review

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:40:57 +00:00 (Migrated from github.com)
Review

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
haythem96 commented 2019-08-03 14:42:56 +00:00 (Migrated from github.com)
Review

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
raucao commented 2019-08-03 14:46:37 +00:00 (Migrated from github.com)
Review

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-06 10:37:30 +00:00 (Migrated from github.com)
Review

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:54:48 +00:00 (Migrated from github.com)
Review

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:55:23 +00:00 (Migrated from github.com)
Review

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
haythem96 commented 2019-08-09 22:56:07 +00:00 (Migrated from github.com)
Review

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
raucao commented 2019-09-19 09:19:32 +00:00 (Migrated from github.com)
Review

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.