WIP: Contribution contracts tests #159

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

View File

@ -181,14 +181,32 @@ 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.
// eslint-disable-next-line no-undef
assert.equal(mintTokenPermission, true);
});
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.
});
describe("Add contribution", async () => {
let amount = 100;
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 contributorId = 1;
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 hashDigest = '0x0000000000000000000000000000000000000000000000000000000000000000';
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 hashFunction = 1;
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 hashSize = 1;
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.
// contributor detials
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 account, contributorHashDigest, contributorHashFunction, contributorHashSize;
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.
// contribution details
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 amount, contributorId, hashDigest, hashFunction, hashSize;
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.
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.
// 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
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.
before(async () => {
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.
// Add contributor from Contributor app
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.
account = root;
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.
contributorHashDigest = '0x0000000000000000000000000000000000000000000000000000000000000000';
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.
contributorHashFunction = 0;
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.
contributorHashSize = 0;
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 contributor.addContributor(account, contributorHashDigest, contributorHashFunction, contributorHashSize);
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.
// 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
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.
assert.equal(await contributor.addressExists(account), true);
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.
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.
amount = 100;
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.
contributorId = await contributor.getContributorIdByAddress(root);
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.
hashDigest = '0x0000000000000000000000000000000000000000000000000000000000000000';
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.
hashFunction = 1;
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.
hashSize = 1;
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.
});
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.
it("should revert when add contribution from address that does not have permission", async () => {
return assertRevert(async () => {
@ -212,7 +230,7 @@ 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.
assert.equal(contributionCountAfter.toNumber()-contributionCountBefore.toNumber(), 1, "contributions counter incremented");
let contributionObject = await contribution.getContribution(contributionCountAfter.toNumber());
// eslint-disable-next-line no-undef
assert.equal(contributionObject[1], contributorId, "contribution added belong to contributor id");
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.
assert.equal(contributionObject[1].toNumber(), contributorId.toNumber(), "contribution added belong to contributor id");
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 isExist = await contribution.exists(contributionCountAfter.toNumber());
// eslint-disable-next-line no-undef
assert.equal(isExist, true, "contribution exist");
@ -258,15 +276,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.
// eslint-disable-next-line no-undef
before(async () =>{
//add contributor
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 account = root;
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 contributorHashDigest = '0x0000000000000000000000000000000000000000000000000000000000000000';
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 contributorHashFunction = 0;
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 contributorHashSize = 0;
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 contributor.addContributor(account, contributorHashDigest, contributorHashFunction, contributorHashSize);
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.
// 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
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.
assert.equal(await contributor.addressExists(account), true);
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.
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.
//add contribution
let amount = 200;
let contributorId = await contributor.getContributorIdByAddress(root);
@ -294,14 +303,20 @@ 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("claim contribution", async () => {
let contributionObject = await contribution.getContribution(contributionId.toNumber());
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 confirmationBlock = 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.
let chainBlockNumberBefore = await 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.
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.
if(contributionId > 10) {
await timeTravel(blocksToWait);
await mineBlock();
let chainBlockNumberAfter = await 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.
// 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
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.
assert.equal(chainBlockNumberAfter.toNumber()-chainBlockNumberBefore.toNumber(), confirmationBlock.toNumber());
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.
}
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.
//Claim contribution
await contribution.claim(contributionId);
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 contributionObject = await contribution.getContribution(contributionId.toNumber());
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, {from: root});
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.
contributionObject = await contribution.getContribution(contributionId.toNumber());
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.
// eslint-disable-next-line no-undef
assert(contributionObject[3], true);
});

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.