Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Court accounting #67

Merged
merged 7 commits into from
Aug 5, 2019
Merged

Court accounting #67

merged 7 commits into from
Aug 5, 2019

Conversation

facuspagnuolo
Copy link
Contributor

This PR introduces the concept of CourtAccounting, a new contract responsible for holding funds related to fees and handling withdraws to the corresponding holders.

@facuspagnuolo facuspagnuolo self-assigned this Jul 25, 2019
@facuspagnuolo facuspagnuolo changed the base branch from split_final_round_3 to split_staking_3 July 25, 2019 11:58
@facuspagnuolo facuspagnuolo force-pushed the court_accounting branch 2 times, most recently from bce908b to ae8c556 Compare July 25, 2019 13:53
@facuspagnuolo facuspagnuolo force-pushed the court_accounting branch 3 times, most recently from 40d63c9 to c12b793 Compare July 27, 2019 13:12
Copy link
Contributor

@izqui izqui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

owner = _owner;
}

function deposit(ERC20 _token, address _to, uint256 _amount) external onlyOwner {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function name is a bit strange given that the actual deposit of tokens in this contract must have already happened. I would rename it to assign or debit as what we are doing is just allowing the receiver to withdraw tokens in the future.

@@ -802,7 +808,8 @@ contract Court is IStakingOwner, ICRVotingOwner, ISubscriptionsOwner {
terms[_draftTermId].dependingDrafts += 1;

if (_feeAmount > 0) {
require(_feeToken.safeTransferFrom(msg.sender, address(staking), _feeAmount), ERROR_DEPOSIT_FAILED);
// TODO: can we do the transfer-from from the accounting itself? the disputer must approve the accounting contract in that case
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is cleaner to approve the same contract that you interact with as it is right now, but either way is fine with me.

@facuspagnuolo facuspagnuolo changed the base branch from split_staking_3 to development August 5, 2019 18:01
@facuspagnuolo facuspagnuolo merged commit f8802bc into development Aug 5, 2019
@delete-merged-branch delete-merged-branch bot deleted the court_accounting branch August 5, 2019 22:14
@izqui izqui added this to the Freeze #1 milestone Oct 15, 2019
facuspagnuolo added a commit that referenced this pull request Oct 12, 2020
* disputes: handle agreement metadata

* disputes: fix agreements metadata handler

* disputes: fix agreements metadata handler

* disputes: use app ID for agreement dispute metadata header

* disputes: allow disputable org to be null

* disputes: fix agreement content mapping
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants