Add Reimbursement app #198

Merged
bumi merged 21 commits from feature/expenses into master 2021-06-02 12:25:34 +00:00
5 changed files with 13 additions and 14 deletions
Showing only changes of commit c4e7e1259e - Show all commits

View File

@ -8,7 +8,7 @@ contract Reimbursement is AragonApp {
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
bytes32 public constant VETO_REIMBURSEMENT_ROLE = keccak256("VETO_REIMBURSEMENT_ROLE");
struct ReimbursementData {
address requestedBy;
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
address recordedBy;
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
uint32 contributorId;
uint256 amount;
address token;
@ -44,12 +44,12 @@ contract Reimbursement is AragonApp {
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
}
}
function get(uint32 reimbursementId) public view returns (uint32 id, address requestedBy, uint32 contributorId, uint256 amount, address token, bytes32 hashDigest, uint8 hashFunction, uint8 hashSize, uint256 confirmedAtBlock, bool exists, bool vetoed) {
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
function get(uint32 reimbursementId) public view returns (uint32 id, address recordedBy, uint32 contributorId, uint256 amount, address token, bytes32 hashDigest, uint8 hashFunction, uint8 hashSize, uint256 confirmedAtBlock, bool exists, bool vetoed) {
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
id = reimbursementId;
ReimbursementData storage r = reimbursements[id];
return (
id,
r.requestedBy,
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
r.recordedBy,
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
r.contributorId,
r.amount,
r.token,
@ -65,7 +65,7 @@ contract Reimbursement is AragonApp {
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
function add(uint256 amount, address token, uint32 contributorId, bytes32 hashDigest, uint8 hashFunction, uint8 hashSize) public isInitialized auth(ADD_REIMBURSEMENT_ROLE) {
uint32 reimbursementId = reimbursementsCount + 1;
ReimbursementData storage r = reimbursements[reimbursementId];
r.requestedBy = msg.sender;
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
r.recordedBy = msg.sender;
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
r.exists = true;
r.amount = amount;
r.token = token;

raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
  event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);

(Although I'm not sure if something like submittedBy isn't nicer for a name there.)

```suggestion event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount); ``` (Although I'm not sure if something like `submittedBy` isn't nicer for a name there.)
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:01:08 +00:00 (Migrated from github.com)
Review

Maybe we should just call this get, analog to add below?

Maybe we should just call this `get`, analog to `add` below?
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
raucao commented 2020-05-29 09:05:54 +00:00 (Migrated from github.com)
Review

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).

I think until we actually have the financial code done, we should not add this function or the `claimed` property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably similar to the contributions, and maybe even combined with dividend payouts, so you only pay for a single tx when withdrawing everything at once).
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
bumi commented 2020-05-29 09:31:35 +00:00 (Migrated from github.com)
Review

yeah, thought about that, too. but we had getContribution in the contribution contract.
do you think that's the best pattern? 👍

yeah, thought about that, too. but we had `getContribution` in the contribution contract. do you think that's the best pattern? :+1:
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
raucao commented 2020-05-29 09:34:07 +00:00 (Migrated from github.com)
Review

Yeah, I think it should be consistent, and contribution.add is nicer for clients than contribution.addContribution. May as well imply the scope for all functions then. (We actually throw a deprecation warning for addContribution still. :))

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))

View File

@ -31,16 +31,14 @@ const contractCalls = [
wiki_username: 'Manuel',
}, { gasLimit: 200000 }]],
['Proposal', 'add', [{ contributorId: 1, contributorIpfsHash: 'QmWKCYGr2rSf6abUPaTYqf98urvoZxGrb7dbspFZA6oyVF', date: '2019-04-09', amount: 500, kind: 'dev', description: '[67P/kredits-contracts] Ran the seeds', url: '' }, { gasLimit: 350000 }]],
['Proposal', 'add', [{ contributorId: 2, contributorIpfsHash: 'QmcHzEeAM26HV2zHTf5HnZrCtCtGdEccL5kUtDakAB7ozB', date: '2019-04-10', amount: 500, kind: 'dev', description: '[67P/kredits-contracts] Ran the seeds', url: '' }, { gasLimit: 350000 }]],
['Proposal', 'add', [{ contributorId: 2, contributorIpfsHash: 'QmcHzEeAM26HV2zHTf5HnZrCtCtGdEccL5kUtDakAB7ozB', date: '2019-04-11', amount: 500, kind: 'dev', description: '[67P/kredits-contracts] Hacked on kredits', url: '' }, { gasLimit: 350000 }]],
['Proposal', 'vote', [1, { gasLimit: 550000 }]],
['Contribution', 'add', [{ contributorId: 1, contributorIpfsHash: 'QmWKCYGr2rSf6abUPaTYqf98urvoZxGrb7dbspFZA6oyVF', date: '2019-04-11', amount: 5000, kind: 'dev', description: '[67P/kredits-contracts] Introduce contribution token', url: '' }, { gasLimit: 350000 }]],
['Contribution', 'add', [{ contributorId: 1, contributorIpfsHash: 'QmWKCYGr2rSf6abUPaTYqf98urvoZxGrb7dbspFZA6oyVF', date: '2019-04-11', amount: 500, kind: 'dev', description: '[67P/kredits-contracts] Test this thing', url: '' }, { gasLimit: 350000 }]],
['Contribution', 'add', [{ contributorId: 2, contributorIpfsHash: 'QmcHzEeAM26HV2zHTf5HnZrCtCtGdEccL5kUtDakAB7ozB', date: '2019-04-11', amount: 1500, kind: 'dev', description: '[67P/kredits-web] Reviewed stuff', url: '' }, { gasLimit: 350000 }]],
['Contribution', 'claim', [1, { gasLimit: 300000 }]],
['Contribution', 'add', [{ contributorId: 1, contributorIpfsHash: 'QmWKCYGr2rSf6abUPaTYqf98urvoZxGrb7dbspFZA6oyVF', date: '2019-04-11', amount: 1500, kind: 'dev', description: '[67P/kredits-contracts] Add tests', url: '' }, { gasLimit: 350000 }]],
['Contribution', 'add', [{ contributorId: 1, contributorIpfsHash: 'QmWKCYGr2rSf6abUPaTYqf98urvoZxGrb7dbspFZA6oyVF', date: '2019-04-11', amount: 1500, kind: 'dev', description: '[67P/kredits-contracts] Introduce contribution token', url: '' }, { gasLimit: 350000 }]],
raucao commented 2020-05-29 09:10:25 +00:00 (Migrated from github.com)
Review

This seems inconsistent with the expense schema. I don't see how it creates the array of expenses, separate from the WBTC reimbursement. Would also be good to add two expenses for an example reimbursement from the start (ideally just the same example as in the schemas repo) for UI development.

This seems inconsistent with the expense schema. I don't see how it creates the array of expenses, separate from the WBTC reimbursement. Would also be good to add two expenses for an example reimbursement from the start (ideally just the same example as in the schemas repo) for UI development.
bumi commented 2020-05-29 09:30:06 +00:00 (Migrated from github.com)
Review

the array here is not an array of expenses but just how it is passed to the add function. see above the seeds for contributions.

The data here is not correct yet. and yep, we can add more just by c&p this line.

the array here is not an array of expenses but just how it is passed to the `add` function. see above the seeds for contributions. The data here is not correct yet. and yep, we can add more just by c&p this line.
raucao commented 2020-05-29 09:36:52 +00:00 (Migrated from github.com)
Review

the array here is not an array of expenses but just how it is passed to the add function. see above the seeds for contributions.

Yes, that's not what I meant. I meant that a reimbursement contains an array of expenses, so it needs be an array for the add function and the examples and seeds as well.

> the array here is not an array of expenses but just how it is passed to the `add` function. see above the seeds for contributions. Yes, that's not what I meant. I meant that a reimbursement contains an array of expenses, so it needs be an array for the `add` function and the examples and seeds as well.
['Contribution', 'add', [{ contributorId: 2, contributorIpfsHash: 'QmcHzEeAM26HV2zHTf5HnZrCtCtGdEccL5kUtDakAB7ozB', date: '2019-04-11', amount: 5000, kind: 'dev', description: '[67P/kredits-web] Expense UI, first draft', url: '' }, { gasLimit: 350000 }]],
['Reimbursement', 'add', [{amount: 100, contributorId: 1, token: '0xa3048576e296207eb0141f2803590ad044f81928', expenses: [{title: 'Server Hosting', description: 'All the serverz', amount: 100, currency: 'EUR', date: '2020-05-28'}]}, { gasLimit: 300000 }]],
['Reimbursement', 'add', [{amount: 10, contributorId: 1, token: '0xa3048576e296207eb0141f2803590ad044f81928', expenses: [{title: 'Domain', description: 'All the domain', amount: 10, currency: 'EUR', date: '2020-05-28'}]}, { gasLimit: 300000 }]],
['Reimbursement', 'add', [{amount: 10, contributorId: 2, token: '0xa3048576e296207eb0141f2803590ad044f81928', expenses: [{title: 'Domain', description: 'All the domain', amount: 10, currency: 'EUR', date: '2020-05-28'}]}, { gasLimit: 300000 }]],
];
const funds = [

View File

@ -1,3 +1,4 @@
raucao commented 2020-05-29 09:11:05 +00:00 (Migrated from github.com)
Review

Are these supposed to be committed?

Are these supposed to be committed?
raucao commented 2020-05-29 09:11:05 +00:00 (Migrated from github.com)
Review

Are these supposed to be committed?

Are these supposed to be committed?
bumi commented 2020-05-29 09:27:33 +00:00 (Migrated from github.com)
Review

arg, noo. of course not.

arg, noo. of course not.
bumi commented 2020-05-29 09:27:33 +00:00 (Migrated from github.com)
Review

arg, noo. of course not.

arg, noo. of course not.
bumi commented 2020-06-27 22:09:47 +00:00 (Migrated from github.com)
Review
  "4": "0x76e069b47b79442657eaf0555a32c6b16fa1b8b4"
```suggestion "4": "0x76e069b47b79442657eaf0555a32c6b16fa1b8b4" ```
bumi commented 2020-06-27 22:09:47 +00:00 (Migrated from github.com)
Review
  "4": "0x76e069b47b79442657eaf0555a32c6b16fa1b8b4"
```suggestion "4": "0x76e069b47b79442657eaf0555a32c6b16fa1b8b4" ```
{
"4": "0x76e069b47b79442657eaf0555a32c6b16fa1b8b4"
raucao commented 2020-05-29 09:11:05 +00:00 (Migrated from github.com)
Review

Are these supposed to be committed?

Are these supposed to be committed?
bumi commented 2020-05-29 09:27:33 +00:00 (Migrated from github.com)
Review

arg, noo. of course not.

arg, noo. of course not.
bumi commented 2020-06-27 22:09:47 +00:00 (Migrated from github.com)
Review
  "4": "0x76e069b47b79442657eaf0555a32c6b16fa1b8b4"
```suggestion "4": "0x76e069b47b79442657eaf0555a32c6b16fa1b8b4" ```
"4": "0x76e069b47b79442657eaf0555a32c6b16fa1b8b4",
raucao commented 2020-05-29 09:11:05 +00:00 (Migrated from github.com)
Review

Are these supposed to be committed?

Are these supposed to be committed?
bumi commented 2020-05-29 09:27:33 +00:00 (Migrated from github.com)
Review

arg, noo. of course not.

arg, noo. of course not.
bumi commented 2020-06-27 22:09:47 +00:00 (Migrated from github.com)
Review
  "4": "0x76e069b47b79442657eaf0555a32c6b16fa1b8b4"
```suggestion "4": "0x76e069b47b79442657eaf0555a32c6b16fa1b8b4" ```
"69041181": "0x6Ad8CDF71C3E2AaD2712964097b475184a0dfDeF"
raucao commented 2020-05-29 09:11:05 +00:00 (Migrated from github.com)
Review

Are these supposed to be committed?

Are these supposed to be committed?
bumi commented 2020-05-29 09:27:33 +00:00 (Migrated from github.com)
Review

arg, noo. of course not.

arg, noo. of course not.
bumi commented 2020-06-27 22:09:47 +00:00 (Migrated from github.com)
Review
  "4": "0x76e069b47b79442657eaf0555a32c6b16fa1b8b4"
```suggestion "4": "0x76e069b47b79442657eaf0555a32c6b16fa1b8b4" ```
}

View File

@ -1,3 +1,4 @@
bumi commented 2020-06-27 22:09:00 +00:00 (Migrated from github.com)
Review
  "4": "0xc34edf7d11b7f8433d597f0bb0697acdff55ef14"
```suggestion "4": "0xc34edf7d11b7f8433d597f0bb0697acdff55ef14" ```
bumi commented 2020-06-27 22:09:00 +00:00 (Migrated from github.com)
Review
  "4": "0xc34edf7d11b7f8433d597f0bb0697acdff55ef14"
```suggestion "4": "0xc34edf7d11b7f8433d597f0bb0697acdff55ef14" ```
{
"4": "0xc34edf7d11b7f8433d597f0bb0697acdff55ef14"
bumi commented 2020-06-27 22:09:00 +00:00 (Migrated from github.com)
Review
  "4": "0xc34edf7d11b7f8433d597f0bb0697acdff55ef14"
```suggestion "4": "0xc34edf7d11b7f8433d597f0bb0697acdff55ef14" ```
"4": "0xc34edf7d11b7f8433d597f0bb0697acdff55ef14",
bumi commented 2020-06-27 22:09:00 +00:00 (Migrated from github.com)
Review
  "4": "0xc34edf7d11b7f8433d597f0bb0697acdff55ef14"
```suggestion "4": "0xc34edf7d11b7f8433d597f0bb0697acdff55ef14" ```
"69041181": "0x356685A0d9d66d6e5dA80f50EC206Af8009C8b6F"
bumi commented 2020-06-27 22:09:00 +00:00 (Migrated from github.com)
Review
  "4": "0xc34edf7d11b7f8433d597f0bb0697acdff55ef14"
```suggestion "4": "0xc34edf7d11b7f8433d597f0bb0697acdff55ef14" ```
}

View File

@ -1,5 +1,4 @@
let schemas = require('@kosmos/schemas');
schemas['expense'] = require('@kosmos/schemas/schemas/expense.json');
const validator = require('../utils/validator');
/**