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

feat: allow debtor consent delegation in DebtKernel #184

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
18 changes: 17 additions & 1 deletion contracts/DebtKernel.sol
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ contract DebtKernel is Pausable {
mapping (bytes32 => bool) public issuanceCancelled;
mapping (bytes32 => bool) public debtOrderCancelled;

mapping (address => address) public debtorConsentDelegation;
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels odd to just do this for debtors and not for other actors in the protocol — in particular, I think there are many reasons this could be useful for creditors as well. Would pretty dramatically simplify the way creditor proxies work, for instance.


event LogDebtOrderFilled(
bytes32 indexed _agreementId,
uint _principal,
Expand Down Expand Up @@ -292,6 +294,14 @@ contract DebtKernel is Pausable {
LogDebtOrderCancelled(debtOrder.debtOrderHash, msg.sender);
}

function delegateDebtorConsent(
address delegate
)
public
{
debtorConsentDelegation[msg.sender] = delegate;
}

////////////////////////
// INTERNAL FUNCTIONS //
////////////////////////
Expand Down Expand Up @@ -337,8 +347,14 @@ contract DebtKernel is Pausable {
{
// Invariant: debtor's signature must be valid, unless debtor is submitting order
chrismin marked this conversation as resolved.
Show resolved Hide resolved
if (msg.sender != debtOrder.issuance.debtor) {
address delegatedDebtor = debtOrder.issuance.debtor;

if (debtorConsentDelegation[debtOrder.issuance.debtor] != address(0)) {
delegatedDebtor = debtorConsentDelegation[debtOrder.issuance.debtor];
}

if (!isValidSignature(
debtOrder.issuance.debtor,
delegatedDebtor,
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach only delegates the ability for the given delegate to sign on the debtor's behalf, but not the ability to more broadly do anything on the debtor's behalf that would require the debtor's consent in some way. IMO this delegation should be more expansive — it should be possible, for instance, for the delegate to be a contract (which can't sign messages, but can submit them as a msg.sender).

I'd recommend putting the delegation logic outside of the if (msg.sender... check and including the delegatedDebtor in that logic in that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about delegation of consent more broadly, and agree that it would make for a much more expressive DebtKernel.

My main worry is about unnecessarily expanding scope to beyond what we need today and potentially introducing bugs, vulnerabilities, etc.

U think we're moving towards a world where these contracts (core settlement contracts + dharma lever contracts) are more upgradeable, which is why I'd advocate with moving forward with the minimum set of work required to get this working for trustless relay.

If you guys feel that there is imminent feature work that will leverage consent delegation for creditors at the DebtKernel level, etc., I'm more than happy to make those code changes.

cc: @graemecode

debtOrder.debtOrderHash,
signaturesV[0],
signaturesR[0],
Expand Down