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: dcurve #91

Merged
merged 25 commits into from
Aug 25, 2021
Merged

feat: dcurve #91

merged 25 commits into from
Aug 25, 2021

Conversation

thelostone-mc
Copy link
Collaborator

@thelostone-mc thelostone-mc commented Aug 9, 2021

@dgrants/dcurve

This package allows you to create a distribution for a specific GrantRound based on the contributions recieved by the Grants.

This package is intended to be used

  • by the dApp hosting dGrants to
    • generate the CLR distribution at the end of the round
    • how the prediction would vary for a grant it it were to receive an additional contribution of value X
  • as a standalone dApp to allow members of the DAO to verify the distribution/merkle root themselves to ensure that the results are correct

Structure

.
├── src
│   ├── README.md               # getting started guide
│   ├── types.ts                # typescript types
|   ├── index.ts                # root of the project, exporting public interface
│   ├── internal
│       ├── calc                # folder containing implementations of CLR algorithms
│       ├── merkle.ts           # file containing implementations to generate merkle-root and claims
|       ├── fetch.ts            # fetch information from chain
|       ├── clr.ts              # orchestrator
└── ...

Examples/Testing

The distribution calculation has been (inefficiently) added to the datastore (app/src/store/data.ts) so that we can test the functionality ahead of getting this merged - please visit any page and check the console.

@frankchen07
Copy link
Contributor

@thelostone-mc - in linear.ts starting on line 68:

    if (grantMatch.match >= matchCap) {
      // normalize match if grant has match greater than cap
      grantMatch.match = matchCap;
    }
  });

What's the use for match cap here? I think that was from downtown stimulus - we're not artificially limiting the cap, but we are normalizing when we hit saturation.

@frankchen07
Copy link
Contributor

The rest of the CLR logic LGTM. 👍

@gdixon gdixon force-pushed the dcurve branch 3 times, most recently from 9cf2420 to c626dae Compare August 19, 2021 18:49
@gdixon gdixon marked this pull request as ready for review August 19, 2021 18:50
@gdixon gdixon force-pushed the dcurve branch 2 times, most recently from 6373349 to d835977 Compare August 19, 2021 21:09
@gdixon
Copy link
Member

gdixon commented Aug 19, 2021

All changes are in!

Everything is working correctly, the fetch method may benefit from another set of eyes (@mds1?), in the future we will probably replace the rpc calls with queries to the graph, but for now I was wondering if we could better filter the GrantDonation's to pull down only a specific rounds events?

If we're happy to optimise later, then this PR is good for review 🙌 @apbendi @phutchins @mds1 @frankchen07 @corydickson @wildmolasses @jferas - any feedback, please let us know and we'll get it sorted asap! 👍

Thanks for all your help here @thelostone-mc!!

mds1
mds1 previously requested changes Aug 20, 2021
Copy link
Contributor

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

This is awesomeeeeeee great job! Love all the code comments too, really helped me understand what's going on 😍

Personally I like avoiding reliance on the Graph for as long as possible because it does add another dependency / point of failure. Added some comments about the query options, and one other thing is we can cache events in IndexedDB if we want (caching in localStorage will fill it up too fast)

app/vite.config.ts Outdated Show resolved Hide resolved
dcurve/README.md Outdated Show resolved Hide resolved
dcurve/README.md Outdated Show resolved Hide resolved
dcurve/README.md Outdated Show resolved Hide resolved
Comment on lines +18 to +20
"dev": "echo 'TODO'",
"build": "echo 'TODO'",
"test": "echo 'TODO'",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these intentionally left as TODO for now? Also I don't see any tests, but this feels like a good package for testing since we want to make sure the results are accurate. But we can create an issue for that and handle it at some future date

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup ! we'd want to add this in soon

Copy link
Contributor

Choose a reason for hiding this comment

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

@thelostone-mc Is there an issue for this? Just looked but might have missed it

Copy link
Member

Choose a reason for hiding this comment

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

@mds1 - I've just opened #137 and added some questions there to start the conversation - it might be better to track this in a discussion until we iron out the details though?

dcurve/src/internal/fetch.ts Outdated Show resolved Hide resolved
dcurve/src/internal/fetch.ts Outdated Show resolved Hide resolved
dcurve/src/internal/merkle.ts Outdated Show resolved Hide resolved
dcurve/src/internal/merkle.ts Outdated Show resolved Hide resolved
dcurve/src/internal/utils.ts Outdated Show resolved Hide resolved
// get contributions and grantRound details
const grantRoundArgs = await fetch({
provider: provider.value,
grantRound: '0x8B4091997E3EbB87be90cEd3e50d8Bb27e1DC742',
Copy link
Contributor

Choose a reason for hiding this comment

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

kind of a nit, just wondering why it's not a constant here.

Copy link
Member

Choose a reason for hiding this comment

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

This was just to demo/test that things are working, we will likely want to hook the prediction call in rather than calculate and we can probably avoid calling fetch from the app itself too.

We can remove these calls prior to merging 🙌

Copy link
Contributor

@phutchins phutchins left a comment

Choose a reason for hiding this comment

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

All looks good!

distribution.hash = getMerkleRoot(distribution.merkle);
} else {
// empty state when we can't create a tree
distribution.merkleError = 'Missing required context to build tree';
Copy link
Contributor

Choose a reason for hiding this comment

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

I could definitely see this being used in a DAO UI as its something that is very much needed. It may be a while before we have that however so I could see going either way for the short term.

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.

6 participants