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 16 additions and 13 deletions
Showing only changes of commit 2c567fa71a - Show all commits

View File

@ -8,7 +8,8 @@ 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 recipient;
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 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. :))
uint32 contributorId;
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. :))
uint256 amount;
address token;
bytes32 hashDigest;
@ -43,12 +44,13 @@ 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 recipient, 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 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. :))
id = reimbursementId;
ReimbursementData storage r = reimbursements[id];
return (
id,
r.recipient,
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.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.contributorId,
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.amount,
r.token,
r.hashDigest,
@ -60,13 +62,14 @@ 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, address recipient, bytes32 hashDigest, uint8 hashFunction, uint8 hashSize) public isInitialized auth(ADD_REIMBURSEMENT_ROLE) {
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 add(uint256 amount, address token, uint32 contributorId, bytes32 hashDigest, uint8 hashFunction, uint8 hashSize) public isInitialized auth(ADD_REIMBURSEMENT_ROLE) {
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 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.exists = true;
r.amount = amount;
r.token = token;
r.recipient = recipient;
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 = contributorId;
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.hashDigest = hashDigest;
r.hashFunction = hashFunction;
r.hashSize = hashSize;

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

@ -39,8 +39,8 @@ const contractCalls = [
['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 }]],
['Reimbursement', 'add', [{amount: 100, recipient: '0x7e8f313c56f809188313aa274fa67ee58c31515d', token: '0xa3048576e296207eb0141f2803590ad044f81928', expenses: [{title: 'Server Hosting', description: 'All the serverz', amount: 100, currency: 'EUR', date: '2020-05-28'}]}, { gasLimit: 300000 }]],
['Reimbursement', 'add', [{amount: 10, recipient: '0xa502eb4021f3b9ab62f75b57a94e1cfbf81fd827', token: '0xa3048576e296207eb0141f2803590ad044f81928', expenses: [{title: 'Domain', description: 'All the domain', amount: 10, currency: 'EUR', date: '2020-05-28'}]}, { gasLimit: 300000 }]],
['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 }]],
];
const funds = [

File diff suppressed because one or more lines are too long

View File

@ -23,10 +23,10 @@ class Reimbursement extends Record {
async add (attrs, callOptions = {}) {
const amount = parseInt(attrs.amount);
const token = attrs.token;
const recipient = attrs.recipient;
const contributorId = attrs.contributorId;
const expenses = attrs.expenses.map( e => new ExpenseSerializer(e) );
if (!amount > 0 || !token || token === '' || !recipient || recipient === '' || !expenses.length > 0) {
if (!amount > 0 || !token || token === '' || !contributorId || contributorId === '' || !expenses.length > 0) {
return Promise.reject(new Error('Invalid data. amount, token, expenses is required.'));
}
@ -39,7 +39,7 @@ class Reimbursement extends Record {
const reimbursement = [
amount,
token,
recipient,
contributorId,
ipfsHashAttr.hashDigest,
ipfsHashAttr.hashFunction,
ipfsHashAttr.hashSize,

View File

@ -15,7 +15,7 @@ module.exports = async function(callback) {
console.log(`Using Reimbursement at: ${kredits.Reimbursement.contract.address}`);
const table = new Table({
head: ['ID', 'Amount', 'Token', 'Recipent', 'Confirmed?', 'Vetoed?', 'IPFS', 'Expenses']
head: ['ID', 'Amount', 'Token', 'ContributorId', 'Confirmed?', 'Vetoed?', 'IPFS', 'Expenses']
})
try {
@ -31,7 +31,7 @@ module.exports = async function(callback) {
r.id.toString(),
r.amount.toString(),
`${r.token}`,
`${r.recipient}`,
`${r.contributorId}`,
`${confirmed}`,
`${r.vetoed}`,
`${r.ipfsHash}`,