Add Reimbursement app #198

Merged
bumi merged 21 commits from feature/expenses into master 2021-06-02 12:25:34 +00:00
33 changed files with 9301 additions and 41 deletions

View File

@ -21,8 +21,8 @@ contract Contribution is AragonApp {
bytes32 public constant KERNEL_APP_ADDR_NAMESPACE = 0xd6f028ca0e8edb4a8c9757ca4fdccab25fa1e0317da1188108f7d2dee14902fb;
// ensure alphabetic order
enum Apps { Contribution, Contributor, Proposal, Token }
bytes32[4] public appIds;
enum Apps { Contribution, Contributor, Proposal, Reimbursement, Token }
bytes32[5] public appIds;
struct ContributionData {
uint32 contributorId;
@ -54,7 +54,7 @@ contract Contribution is AragonApp {
event ContributionClaimed(uint32 id, uint32 indexed contributorId, uint32 amount);
event ContributionVetoed(uint32 id, address vetoedByAccount);
function initialize(bytes32[4] _appIds) public onlyInit {
function initialize(bytes32[5] _appIds) public onlyInit {
appIds = _appIds;
blocksToWait = 40320; // 7 days; 15 seconds block time
initialized();

View File

@ -28,14 +28,14 @@ contract Contributor is AragonApp {
uint32 public contributorsCount;
// ensure alphabetic order
enum Apps { Contribution, Contributor, Proposal, Token }
bytes32[4] public appIds;
enum Apps { Contribution, Contributor, Proposal, Reimbursement, Token }
bytes32[5] public appIds;
event ContributorProfileUpdated(uint32 id, bytes32 oldHashDigest, bytes32 newHashDigest); // what should be logged
event ContributorAccountUpdated(uint32 id, address oldAccount, address newAccount);
event ContributorAdded(uint32 id, address account);
function initialize(address root,bytes32[4] _appIds) public onlyInit {
function initialize(address root, bytes32[5] _appIds) public onlyInit {
appIds = _appIds;
initialized();

View File

@ -20,8 +20,8 @@ contract Proposal is AragonApp {
bytes32 public constant KERNEL_APP_ADDR_NAMESPACE = 0xd6f028ca0e8edb4a8c9757ca4fdccab25fa1e0317da1188108f7d2dee14902fb;
// ensure alphabetic order
enum Apps { Contribution, Contributor, Proposal, Token }
bytes32[4] public appIds;
enum Apps { Contribution, Contributor, Proposal, Reimbursement, Token }
bytes32[5] public appIds;
struct Proposal {
address creatorAccount;
@ -46,7 +46,7 @@ contract Proposal is AragonApp {
event ProposalVoted(uint32 id, uint32 voterId, uint16 totalVotes);
event ProposalExecuted(uint32 id, uint32 contributorId, uint32 amount);
function initialize(bytes32[4] _appIds) public onlyInit {
function initialize(bytes32[5] _appIds) public onlyInit {
appIds = _appIds;
initialized();
}

1
apps/reimbursement/.gitattributes vendored Normal file
View File

@ -0,0 +1 @@
*.sol linguist-language=Solidity

7
apps/reimbursement/.gitignore vendored Normal file
View File

@ -0,0 +1,7 @@
node_modules
artifacts
.cache
cache
dist
ipfs.cmd
package-lock.json

View File

@ -0,0 +1,34 @@
{
"roles": [
{
"name": "Add reimursements",
"id": "ADD_REIMBURSEMENT_ROLE",
"params": []
},
{
"name": "Veto reimbursement",
"id": "VETO_REIMBURSEMENT_ROLE",
"params": []
}
],
"environments": {
"default": {
"network": "development",
"appName": "kredits-reimbursement.open.aragonpm.eth"
},
"rinkeby": {
"registry": "0x98df287b6c145399aaa709692c8d308357bc085d",
"appName": "kredits-reimbursement.open.aragonpm.eth",
"wsRPC": "wss://rinkeby.eth.aragon.network/ws",
"network": "rinkeby"
},
"production": {
"registry": "0x314159265dd8dbb310642f98f50c066173c1259b",
"appName": "kredits-reimbursement.aragonpm.eth",
"wsRPC": "wss://mainnet.eth.aragon.network/ws",
"network": "mainnet"
}
},
"appName": "kredits-reimbursement.aragonpm.eth",
"path": "contracts/Reimbursement.sol"
}

View File

@ -0,0 +1,34 @@
const { usePlugin } = require('@nomiclabs/buidler/config')
const hooks = require('./scripts/buidler-hooks')
usePlugin('@aragon/buidler-aragon')
module.exports = {
// Default Buidler configurations. Read more about it at https://buidler.dev/config/
defaultNetwork: 'localhost',
networks: {
localhost: {
url: 'http://localhost:8545',
},
},
solc: {
version: '0.4.24',
optimizer: {
enabled: true,
runs: 10000,
},
},
// Etherscan plugin configuration. Learn more at https://github.com/nomiclabs/buidler/tree/master/packages/buidler-etherscan
etherscan: {
apiKey: '', // API Key for smart contract verification. Get yours at https://etherscan.io/apis
},
// Aragon plugin configuration
aragon: {
appServePort: 8001,
clientServePort: 3000,
appSrcPath: 'app/',
appBuildOutputPath: 'dist/',
appName: 'expenses',
hooks, // Path to script hooks
},
}

View File

@ -0,0 +1,94 @@
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. :))
pragma solidity ^0.4.24;
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. :))
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. :))
import "@aragon/os/contracts/apps/AragonApp.sol";
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. :))
import "@aragon/os/contracts/kernel/IKernel.sol";
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. :))
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. :))
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 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. :))
bytes32 public constant ADD_REIMBURSEMENT_ROLE = keccak256("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. :))
bytes32 public constant VETO_REIMBURSEMENT_ROLE = keccak256("VETO_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. :))
// bytes32 public constant MANAGE_APPS_ROLE = keccak256("MANAGE_APPS_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. :))
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. :))
struct ReimbursementData {
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 recipientId;
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;
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 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 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. :))
bytes32 hashDigest;
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. :))
uint8 hashFunction;
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. :))
uint8 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 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 confirmedAtBlock;
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. :))
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. :))
bool exists;
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. :))
}
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. :))
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. :))
mapping(uint32 => ReimbursementData) public reimbursements;
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 public reimbursementsCount;
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. :))
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 public blocksToWait;
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. :))
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. :))
event ReimbursementAdded(uint32 id, address indexed addedByAccount, uint256 amount);
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. :))
event ReimbursementVetoed(uint32 id, address vetoedByAccount);
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. :))
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 initialize() public onlyInit {
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. :))
blocksToWait = 40320; // 7 days; 15 seconds block time
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. :))
initialized();
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. :))
}
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. :))
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 setApps() public isInitialized auth(MANAGE_APPS_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. :))
// }
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. :))
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 totalAmount(bool confirmedOnly) public view returns (uint256 amount) {
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. :))
for (uint32 i = 1; i <= reimbursementsCount; i++) {
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. :))
ReimbursementData memory r = reimbursements[i];
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. :))
if (!r.vetoed && (block.number >= r.confirmedAtBlock || !confirmedOnly)) {
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. :))
amount += r.amount; // should use safemath
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. :))
}
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. :))
}
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. :))
}
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. :))
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, uint32 recipientId, 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;
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. :))
ReimbursementData storage r = reimbursements[id];
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. :))
return (
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,
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.recipientId,
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,
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.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 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,
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.hashFunction,
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.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 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.confirmedAtBlock,
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,
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.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. :))
);
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. :))
}
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. :))
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 recipientId, 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;
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. :))
ReimbursementData storage r = reimbursements[reimbursementId];
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;
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 = amount;
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.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 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.recipientId = recipientId;
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;
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.hashFunction = hashFunction;
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.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 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.confirmedAtBlock = block.number + blocksToWait;
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. :))
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. :))
reimbursementsCount++;
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. :))
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. :))
emit ReimbursementAdded(reimbursementId, msg.sender, amount);
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. :))
}
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. :))
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 veto(uint32 reimbursementId) public isInitialized auth(VETO_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. :))
ReimbursementData storage r = reimbursements[reimbursementId];
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. :))
require(r.exists, 'NOT_FOUND');
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. :))
require(block.number < r.confirmedAtBlock, 'VETO_PERIOD_ENDED');
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.vetoed = true;
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. :))
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. :))
emit ReimbursementVetoed(reimbursementId, 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. :))
}
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. :))
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 exists(uint32 reimbursementId) public view returns (bool) {
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. :))
return reimbursements[reimbursementId].exists;
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. :))
}
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. :))
}
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. :))

View File

@ -0,0 +1,16 @@
{
"name": "kredits Reimbursement",
"author": "Placeholder-author",
"description": "An application for Aragon",
"details_url": "/meta/details.md",
"source_url": "https://<placeholder-repository-url>",
"icons": [
{
"src": "/meta/icon.svg",
"sizes": "56x56"
}
],
"screenshots": [{ "src": "/meta/screenshot-1.png" }],
"start_url": "/index.html",
"script": "/script.js"
}

View File

@ -0,0 +1,30 @@
{
"name": "kredits-reimbursement",
"version": "1.0.0",
"description": "",
"scripts": {
"start": "npm run start:aragon:ipfs",
"start:aragon:ipfs": "aragon run",
"start:aragon:http": "aragon run --http localhost:8001 --http-served-from ./dist",
"start:app": "",
"compile": "aragon contracts compile",
"sync-assets": "",
"build:app": "",
"build:script": "",
"build": "",
"publish:patch": "aragon apm publish patch",
"publish:minor": "aragon apm publish minor",
"publish:major": "aragon apm publish major",
"versions": "aragon apm versions",
"test": "truffle test"
},
"dependencies": {
"@aragon/os": "^4.4.0"
},
"devDependencies": {
"@aragon/test-helpers": "^2.1.0",
"eth-gas-reporter": "^0.2.17",
"ganache-cli": "^6.9.1",
"solidity-coverage": "^0.5.11"
}
}

View File

@ -0,0 +1,65 @@
/*
const { hash } = require('eth-ens-namehash')
const { getEventArgument } = require('@aragon/contract-test-helpers/events')
const Kernel = artifacts.require('@aragon/os/build/contracts/kernel/Kernel')
const ACL = artifacts.require('@aragon/os/build/contracts/acl/ACL')
const EVMScriptRegistryFactory = artifacts.require(
'@aragon/os/build/contracts/factory/EVMScriptRegistryFactory'
)
const DAOFactory = artifacts.require(
'@aragon/os/build/contracts/factory/DAOFactory'
)
const newDao = async (rootAccount) => {
// Deploy a DAOFactory.
const kernelBase = await Kernel.new(true)
const aclBase = await ACL.new()
const registryFactory = await EVMScriptRegistryFactory.new()
const daoFactory = await DAOFactory.new(
kernelBase.address,
aclBase.address,
registryFactory.address
)
// Create a DAO instance.
const daoReceipt = await daoFactory.newDAO(rootAccount)
const dao = await Kernel.at(getEventArgument(daoReceipt, 'DeployDAO', 'dao'))
// Grant the rootAccount address permission to install apps in the DAO.
const acl = await ACL.at(await dao.acl())
const APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE()
await acl.createPermission(
rootAccount,
dao.address,
APP_MANAGER_ROLE,
rootAccount,
{ from: rootAccount }
)
return { dao, acl }
}
const newApp = async (dao, appName, baseAppAddress, rootAccount) => {
const receipt = await dao.newAppInstance(
hash(`${appName}.aragonpm.test`), // appId - Unique identifier for each app installed in the DAO; can be any bytes32 string in the tests.
baseAppAddress, // appBase - Location of the app's base implementation.
'0x', // initializePayload - Used to instantiate and initialize the proxy in the same call (if given a non-empty bytes string).
false, // setDefault - Whether the app proxy is the default proxy.
{ from: rootAccount }
)
// Find the deployed proxy address in the tx logs.
const logs = receipt.logs
const log = logs.find((l) => l.event === 'NewAppProxy')
const proxyAddress = log.args.proxy
return proxyAddress
}
module.exports = {
newDao,
newApp,
}
*/

View File

@ -0,0 +1,21 @@
/*
const ANY_ADDRESS = '0xffffffffffffffffffffffffffffffffffffffff'
const setOpenPermission = async (acl, appAddress, role, rootAddress) => {
// Note: Setting a permission to 0xffffffffffffffffffffffffffffffffffffffff
// is interpreted by aragonOS as allowing the role for any address.
await acl.createPermission(
ANY_ADDRESS, // entity (who?) - The entity or address that will have the permission.
appAddress, // app (where?) - The app that holds the role involved in this permission.
role, // role (what?) - The particular role that the entity is being assigned to in this permission.
rootAddress, // manager - Can grant/revoke further permissions for this role.
{ from: rootAddress}
)
}
module.exports = {
setOpenPermission
}
*/

View File

@ -0,0 +1 @@
module.exports = require("../../truffle.js");

8716
apps/reimbursement/yarn.lock Normal file

File diff suppressed because it is too large Load Diff

View File

@ -7,12 +7,12 @@ contract Token is ERC20Token, AragonApp {
bytes32 public constant MINT_TOKEN_ROLE = keccak256("MINT_TOKEN_ROLE");
// ensure alphabetic order
enum Apps { Contribution, Contributor, Proposal, Token }
bytes32[4] public appIds;
enum Apps { Contribution, Contributor, Proposal, Reimbursement, Token }
bytes32[5] public appIds;
event LogMint(address indexed recipient, uint256 amount, uint32 contributionId);
function initialize(bytes32[4] _appIds) public onlyInit {
function initialize(bytes32[5] _appIds) public onlyInit {
appIds = _appIds;
name = 'Kredits';
symbol = '₭S';

View File

@ -31,13 +31,19 @@ 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: 1116000, recipientId: 1, token: '0x2260fac5e5542a773aa44fbcfedf7c193bc2c599', expenses: [
{ title: 'Server rent', description: 'Dedicated server: andromeda.kosmos.org, April 2020', amount: 61, currency: 'EUR', date: '2020-05-28' },
{ title: 'Server rent', description: 'Dedicated server: centaurus.kosmos.org, April 2020', amount: 32, currency: 'EUR', date: '2020-05-28' }
]}, { gasLimit: 300000 }]],
['Reimbursement', 'add', [{amount: 166800, recipientId: 2, token: '0x2260fac5e5542a773aa44fbcfedf7c193bc2c599', expenses: [
{ title: 'Domain kosmos.chat', description: 'Yearly registration fee for domain kosmos.chat', amount: 13.90, currency: 'EUR', date: '2020-05-30' }
]}, { gasLimit: 300000 }]],
];
const funds = [

View File

@ -10,17 +10,18 @@ import "../apps/contribution/contracts/Contribution.sol";
import "../apps/contributor/contracts/Contributor.sol";
import "../apps/token/contracts/Token.sol";
import "../apps/proposal/contracts/Proposal.sol";
import "../apps/reimbursement/contracts/Reimbursement.sol";
contract KreditsKit is KitBase {
// ensure alphabetic order
enum Apps { Contribution, Contributor, Proposal, Token }
bytes32[4] public appIds;
enum Apps { Contribution, Contributor, Proposal, Reimbursement, Token }
bytes32[5] public appIds;
event DeployInstance(address dao);
event InstalledApp(address dao, address appProxy, bytes32 appId);
constructor (DAOFactory _fac, ENS _ens, bytes32[4] _appIds) public KitBase(_fac, _ens) {
constructor (DAOFactory _fac, ENS _ens, bytes32[5] _appIds) public KitBase(_fac, _ens) {
appIds = _appIds;
}
@ -41,18 +42,24 @@ contract KreditsKit is KitBase {
Contribution contribution = Contribution(_installApp(dao, appIds[uint8(Apps.Contribution)]));
contribution.initialize(appIds);
Proposal proposal = Proposal(_installApp(dao, appIds[uint8(Apps.Proposal)]));
proposal.initialize(appIds);
acl.createPermission(root, contribution, contribution.ADD_CONTRIBUTION_ROLE(), this);
acl.createPermission(root, contribution, contribution.VETO_CONTRIBUTION_ROLE(), this);
acl.grantPermission(proposal, contribution, contribution.ADD_CONTRIBUTION_ROLE());
Proposal proposal = Proposal(_installApp(dao, appIds[uint8(Apps.Proposal)]));
proposal.initialize(appIds);
Reimbursement reimbursement = Reimbursement(_installApp(dao, appIds[uint8(Apps.Reimbursement)]));
reimbursement.initialize();
acl.createPermission(root, reimbursement, reimbursement.ADD_REIMBURSEMENT_ROLE(), this);
acl.createPermission(root, reimbursement, reimbursement.VETO_REIMBURSEMENT_ROLE(), this);
uint256[] memory params = new uint256[](1);
params[0] = uint256(203) << 248 | uint256(1) << 240 | uint240(contributor);
acl.grantPermissionP(acl.ANY_ENTITY(), contribution, contribution.ADD_CONTRIBUTION_ROLE(), params);
acl.grantPermissionP(acl.ANY_ENTITY(), contribution, contribution.VETO_CONTRIBUTION_ROLE(), params);
acl.grantPermissionP(acl.ANY_ENTITY(), contributor, contributor.MANAGE_CONTRIBUTORS_ROLE(), params);
acl.grantPermissionP(acl.ANY_ENTITY(), reimbursement, reimbursement.ADD_REIMBURSEMENT_ROLE(), params);
//acl.setPermissionManager(this, proposal, proposal.VOTE_PROPOSAL_ROLE();
acl.createPermission(root, proposal, proposal.VOTE_PROPOSAL_ROLE(), this);
@ -67,6 +74,8 @@ contract KreditsKit is KitBase {
acl.setPermissionManager(root, contribution, contribution.ADD_CONTRIBUTION_ROLE());
acl.setPermissionManager(root, contribution, contribution.VETO_CONTRIBUTION_ROLE());
acl.setPermissionManager(root, contributor, contributor.MANAGE_CONTRIBUTORS_ROLE());
acl.setPermissionManager(root, reimbursement, reimbursement.ADD_REIMBURSEMENT_ROLE());
acl.setPermissionManager(root, reimbursement, reimbursement.VETO_REIMBURSEMENT_ROLE());
acl.createPermission(root, token, token.MINT_TOKEN_ROLE(), this);
acl.grantPermission(contribution, token, token.MINT_TOKEN_ROLE());

File diff suppressed because one or more lines are too long

File diff suppressed because one or more lines are too long

File diff suppressed because one or more lines are too long

File diff suppressed because one or more lines are too long

File diff suppressed because one or more lines are too long

View File

@ -3,6 +3,7 @@ module.exports = {
Contribution: require('./contribution'),
Proposal: require('./proposal'),
Token: require('./token'),
Reimbursement: require('./reimbursement'),
Kernel: require('./kernel'),
Acl: require('./acl'),
};

View File

@ -0,0 +1,65 @@
const Record = require('./record');
const ExpenseSerializer = require('../serializers/expense');
class Reimbursement extends Record {
get count () {
return this.functions.reimbursementsCount();
}
getById (id) {
return this.functions.get(id)
.then(data => {
return this.ipfs.catAndMerge(data, (ipfsDocument) => {
const expenses = JSON.parse(ipfsDocument);
return { expenses };
});
});
}
getData (id) {
return this.functions.getReimbursement(id);
}
async add (attrs, callOptions = {}) {
const amount = parseInt(attrs.amount);
const token = attrs.token;
const recipientId = attrs.recipientId;
const expenses = attrs.expenses.map( e => new ExpenseSerializer(e) );
let errorMessage;
if (typeof amount !== 'number' || amount <= 0) {
errorMessage = 'Invalid data: amount must be a positive number.';
}
if (!token || token === '') {
errorMessage = 'Invalid data: token must be a token address.';
}
if (!recipientId || recipientId === '') {
errorMessage = 'Invalid data: recipientId is required.';
}
if (expenses.length === 0) {
errorMessage = 'Invalid data: at least one expense item is required.';
}
if (errorMessage) { return Promise.reject(new Error(errorMessage)); }
return Promise.all(expenses.map(e => e.validate()))
.then(() => {
const jsonStr = JSON.stringify(expenses.map(e => e.data), null, 2);
return this.ipfs
.add(jsonStr)
.then(ipfsHashAttr => {
const reimbursement = [
amount,
token,
parseInt(recipientId),
ipfsHashAttr.hashDigest,
ipfsHashAttr.hashFunction,
ipfsHashAttr.hashSize,
];
return this.functions.add(...reimbursement, callOptions);
});
});
}
}
module.exports = Reimbursement;

View File

@ -6,6 +6,7 @@ const deprecate = require('./utils/deprecate');
const ABIS = {
Contributor: require('./abis/Contributor.json'),
Contribution: require('./abis/Contribution.json'),
Reimbursement: require('./abis/Reimbursement.json'),
Token: require('./abis/Token.json'),
Proposal: require('./abis/Proposal.json'),
Kernel: require('./abis/Kernel.json'),
@ -16,6 +17,7 @@ const APP_CONTRACTS = [
'Contribution',
'Token',
'Proposal',
'Reimbursement',
'Acl',
];
const DaoAddresses = require('./addresses/dao.json');
@ -121,6 +123,10 @@ class Kredits {
return this.contractFor('Contribution');
}
get Reimbursement () {
return this.contractFor('Reimbursement');
}
get Acl () {
return this.contractFor('Acl');
}

View File

@ -29,7 +29,7 @@ class KreditsKit {
appIdFor (contractName) {
// see appIds in KreditsKit.sol for more details
const knownContracts = ['Contribution', 'Contributor', 'Proposal', 'Token'];
const knownContracts = ['Contribution', 'Contributor', 'Proposal', 'Reimbursement', 'Token'];
return this.contract.appIds(knownContracts.indexOf(contractName));
}

View File

@ -1,4 +1,4 @@
const schemas = require('kosmos-schemas');
const schemas = require('@kosmos/schemas');
const validator = require('../utils/validator');
/**

View File

@ -1,4 +1,4 @@
const schemas = require('kosmos-schemas');
const schemas = require('@kosmos/schemas');
const validator = require('../utils/validator');
/**
* Handle serialization for JSON-LD object of the contributor, according to

100
lib/serializers/expense.js Normal file
View File

@ -0,0 +1,100 @@
const schemas = require('@kosmos/schemas');
const validator = require('../utils/validator');
/**
* Serialization and validation for JSON-LD document of the Expense
*
* @class
* @public
*/
class ExpenseSerializer {
constructor (attrs) {
Object.keys(attrs).forEach(a => this[a] = attrs[a]);
}
/**
* Serialize object to JSON
*
* @public
*/
serialize () {
// Write it pretty to ipfs
return JSON.stringify(this.data, null, 2);
}
get data () {
const {
title,
description,
currency,
amount,
date,
url,
tags,
details,
} = this;
const data = {
'@context': 'https://schema.kosmos.org',
'@type': 'Expense',
title,
description,
currency,
amount,
date,
'tags': tags || [],
'details': details || {},
};
if (url) {
data['url'] = url;
}
return data;
}
/**
* Validate serialized data against schema
*
* @public
*/
validate () {
const serialized = JSON.parse(this.serialize());
const valid = validator.validate(serialized, schemas['expense']);
return valid ? Promise.resolve() : Promise.reject(validator.error);
}
/**
* Deserialize JSON to object
*
* @public
*/
static deserialize (serialized) {
const {
title,
description,
currency,
amount,
date,
url,
tags,
details,
} = JSON.parse(serialized.toString('utf8'));
return {
title,
description,
currency,
amount,
date,
url,
tags,
details,
ipfsData: serialized,
};
}
}
module.exports = ExpenseSerializer;

18
package-lock.json generated
View File

@ -575,6 +575,14 @@
"@ethersproject/strings": "^5.0.0"
}
},
"@kosmos/schemas": {
"version": "3.0.0",
"resolved": "https://registry.npmjs.org/@kosmos/schemas/-/schemas-3.0.0.tgz",
"integrity": "sha512-7hnirq0ShsDjENBPjNnu6izwzVjkDzEZ4SKvPzQJJfkeJ/NRVHKvfdn9rkE7CRXTi2hMzsZVFOnlF6D/eYXTCA==",
"requires": {
"brfs-babel": "^1.0.0"
}
},
"@resolver-engine/core": {
"version": "0.2.1",
"resolved": "https://registry.npmjs.org/@resolver-engine/core/-/core-0.2.1.tgz",
@ -5452,14 +5460,6 @@
"graceful-fs": "^4.1.9"
}
},
"kosmos-schemas": {
"version": "2.2.1",
"resolved": "https://registry.npmjs.org/kosmos-schemas/-/kosmos-schemas-2.2.1.tgz",
"integrity": "sha512-bZRbwLmJVeaaLqIyJsHbUf7wCjgHP2IhKXH12fD5+1tcw6/bdtZF4nZ41SoVCmdnZnSXh6Fv8nIRT0HksSlj8w==",
"requires": {
"brfs-babel": "^1.0.0"
}
},
"ky": {
"version": "0.15.0",
"resolved": "https://registry.npmjs.org/ky/-/ky-0.15.0.tgz",
@ -9916,4 +9916,4 @@
}
}
}
}
}

View File

@ -62,7 +62,7 @@
"dependencies": {
"ethers": "^5.0.2",
"ipfs-http-client": "^41.0.1",
"kosmos-schemas": "^2.2.1",
"@kosmos/schemas": "^3.0.0",
"node-fetch": "^2.6.0",
"tv4": "^1.3.0"
},

View File

@ -11,6 +11,7 @@ const files = [
'Kernel',
'Proposal',
'Token',
'Reimbursement',
'ACL'
];

View File

@ -0,0 +1,52 @@
const promptly = require('promptly');
const Table = require('cli-table');
const initKredits = require('./helpers/init_kredits.js');
module.exports = async function(callback) {
let kredits;
try {
kredits = await initKredits(web3);
} catch(e) {
callback(e);
return;
}
console.log(`Using Reimbursement at: ${kredits.Reimbursement.contract.address}`);
const table = new Table({
head: ['ID', 'Amount', 'Token', 'recipientId', 'Confirmed?', 'Vetoed?', 'IPFS', 'Expenses']
})
try {
let blockNumber = await kredits.provider.getBlockNumber();
let reimbursements = await kredits.Reimbursement.all({page: {size: 1000}});
let kreditsSum = 0;
console.log(`Current block number: ${blockNumber}`);
reimbursements.forEach(r => {
const confirmed = r.confirmedAtBlock <= blockNumber;
table.push([
r.id.toString(),
r.amount.toString(),
`${r.token}`,
`${r.recipientId}`,
`${confirmed}`,
`${r.vetoed}`,
`${r.ipfsHash}`,
`${r.expenses.length}`
]);
});
console.log(table.toString());
let totalAmountUnconfirmed = await kredits.Reimbursement.functions.totalAmount(false);
let totalAmountConfirmed = await kredits.Reimbursement.functions.totalAmount(true);
console.log(`Total: ${totalAmountConfirmed} (confirmed) | ${totalAmountUnconfirmed} (including unconfirmed)`);
} catch (err) {
console.log(err);
}
callback();
}