kredits/contracts
9
1
Fork 0
Code Issues 21 Pull Requests Actions Projects 1 Releases 8 Activity

Add Reimbursement app #198

Merged
bumi merged 21 commits from feature/expenses into master 2021-06-02 12:25:34 +00:00
Conversation 19 Commits 21 Files Changed 33 +2 -2
2 changed files with 2 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 1f248812a7 - Show all commits

2
apps/reimbursement/contracts/Reimbursement.sol
View File

@@ -55,7 +55,7 @@ contract Reimbursement is AragonApp {
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
Copy Link
  event 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
Copy Link
  event 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
Copy Link

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
Copy Link

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
Copy Link

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably 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
Copy Link

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably 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
Copy Link

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
Copy Link

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
Copy Link

Yeah, I think it should be consistent, and contribution.add is nicer 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
Copy Link

Yeah, I think it should be consistent, and contribution.add is nicer 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 getReimbursement(uint32 reimbursementId) public view returns (uint32 id, address recipient, uint256 amount, address token, bool claimed, 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
Copy Link
  event 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
Copy Link

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
Copy Link

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably 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
Copy Link

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
Copy Link

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

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
function get(uint32 reimbursementId) public view returns (uint32 id, address recipient, uint256 amount, address token, bool claimed, 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
Copy Link
  event 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
Copy Link

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
Copy Link

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably 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
Copy Link

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
Copy Link

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

Yeah, I think it should be consistent, and `contribution.add` is nicer for clients than `contribution.addContribution`. May as well imply the scope for all functions then. (We actually throw a deprecation warning for `addContribution` still. :))
id = reimbursementId;
ReimbursementData storage r = reimbursements[id];
return (
raucao commented 2020-05-29 08:54:04 +00:00 (Migrated from github.com)
Review
Copy Link
  event 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
Copy Link
  event 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
Copy Link

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
Copy Link

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
Copy Link

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably 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
Copy Link

I think until we actually have the financial code done, we should not add this function or the claimed property. Both because it will actually be out of sync with what we actually send around from the Gnosis safe, and also because we may want to adopt a different approach here as well (probably 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
Copy Link

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
Copy Link

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
Copy Link

Yeah, I think it should be consistent, and contribution.add is nicer 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
Copy Link

Yeah, I think it should be consistent, and contribution.add is nicer 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. :))

2
lib/contracts/reimbursement.js
View File

@@ -7,7 +7,7 @@ class Reimbursement extends Record {
}
getById (id) {
return this.functions.getReimbursement(id)
return this.functions.get(id)
.then(data => {
return this.ipfs.catAndMerge(data, ExpenseSerializer.deserialize);
});