DIP-1: Creditor-Driven Loans MVP

[bohendo]

This merge request represents the MVP that satisfies DIP-1.

Only the happiest path has had it's units & integration tested so far. We'll wait until we get a green light on the contract itself before we harden the test suite.

Can't wait to start getting some feedback!

Note: Some migration/permission tests are broken because we still haven't received an answer regarding whether to incorporate the creditor proxy into existing migrations or to add a new migration #7. We'll wait until this is clarified before we update pre-existing tests.

[nadavhollander]

🙌🙌🙌 This is amazing! It's on our agenda to get this reviewed ASAP -- hopefully tomorrow at the latest :)

[chrismin]

@bohendo @shivgupt The creditor proxy should have its own migration. If it were to be added to an existing migration X, on environments where migration X has already been run, migration X would not be run at all and your new creditor proxy contract would not get deployed.

[bohendo]

I'd be curious to hear your thoughts on the way we propose using a nonce in DIP-1. (Ignore the fact that it currently reuses the cancelledOrders mapping in way that @chrismin points out is hacky)

Most of the upgrades we propose in the RFC are fairly superficial but the nonce part seems a little more significant and might have a good enough complexity-to-benefit ratio to include in an MVP. It seems like it's logic might be shared between this simple MVP & more flexible methods for filling creditor-driven loans.

[bohendo]

Thank you for all the feedback we've received so far! We've applied your suggestions & have fixed the rest of the broken tests: 895 passing, 0 failing!

Up next:

1. Fill out the sad/error/edge cases & get a final review of this MVP (Migration 7 got a little weird, suggestions for cleaning it up are welcome)
2. See if we can get a PoC going for the more flexible version proposed by a recent comment in DIP-1

Think we'd merge after 1 or after 2?

[bohendo]

We filled out the test suite: 999 tests passing, 0 failing! If anyone wants to suggest one more thing to test it'd be cool to have a 4 figure test suite 😎

We'll be available to take action on any suggestions you guys make. Otherwise, we're gonna shift gears for a bit to focus on EthBerlin and hold off on adding features to the MVP until afterwards.

Can't wait to see @nadavhollander and hopefully a few other Dharma teammates in Berlin!

[nadavhollander]

MUSIC TO MY EARS 🤤

@graemeboy @cminxi @saturnial please prioritize giving Bo and Shivani a PR review!

Nadav Hollander Founder & CEO - Dharma B.S. in Computer Science -- Stanford University 2017 949.293.5907

Sent via Superhuman ( https://sprh.mn/?vip=nadav@dharma.io )

On Sat, Sep 01, 2018 at 11:12 AM, Bo < notifications@github.com > wrote:

We filled out the test suite: 999 tests passing, 0 failing! If anyone wants to suggest one more thing to test it'd be cool to have a 4 figure test suite 😎

We'll be available to take action on any suggestions you guys make. Otherwise, we're gonna shift gears for a bit to focus on EthBerlin and hold off on adding features to the MVP until afterwards.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub ( https://github.com/dharmaprotocol/charta/pull/156#issuecomment-417877610 ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/ADF3XtkfcQ0ZBp8WQhtxpjPdQofhvKzrks5uWs4DgaJpZM4WPDSW ).

[chrismin]

Other than some small comments and a request for one more type of test, LGTM!

Since charta PRs require two LGTMs, I'll ask @graemecode and/or @saturnial to chime in!

[bohendo]

Regarding CirclrCI, shouldn't CI tests be running on potential merge requests? We don't have push access the charta repo itself though so can't create a branch there if that's needed to get the CI tests running.