-
Notifications
You must be signed in to change notification settings - Fork 62
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
Volume matching #406
base: dev
Are you sure you want to change the base?
Volume matching #406
Conversation
We can merge fixed point first. Then we can get this in |
Would require a rebase now |
823b663
to
1660cac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I can review it further, this is missing any comments or documentation explaining what the code does
apps/auctions/volume_matching.py
Outdated
return buys, sells | ||
|
||
|
||
async def volume_matching(ctx, bids): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing comments explaining what this function interface is. What is "bids" supposed to contain? What is the type of it? Are there preconditions? What function does this compute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible try to follow the documentation format guidelines in our contributing guidelines:
https://github.com/initc3/HoneyBadgerMPC/blob/dev/docs/development/contributing.rst#documentation-conventions
Codecov Report
@@ Coverage Diff @@
## dev #406 +/- ##
===================================================
- Coverage 77.02727% 76.78913% -0.23815%
===================================================
Files 50 51 +1
Lines 5611 5743 +132
Branches 859 876 +17
===================================================
+ Hits 4322 4410 +88
- Misses 1113 1157 +44
Partials 176 176 |
We should try to use this PR as an example of the documentation style we want to see in contributed apps |
@lilione when you have a chance, could you rebase the PR so that CircleCI will be used to generate the docs and we can then preview them ... |
@yunqi I added some commits so that you can build and view the docs locally. To build and view the docs in your browser you can run: $ make servedocs To build the docs: $ make docs You then need to refresh the browser. To view the auctions app docs go to http://localhost:58888/apps/auctions.html. Let me know once you are setup! |
apps/auctions/volume_matching.py
Outdated
|
||
Parameters | ||
---------- | ||
balances : {address -> {cointype -> balance}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the type, I think the convention is usually to specify the type, such as list
, dict
, str
, etc. From the numpy docstring docs:
For the parameter types, be as precise as possible. Below are a few examples of parameters and their types.
Parameters ---------- filename : str copy : bool dtype : data-type iterable : iterable object shape : int or tuple of int files : list of str
So in this case that would be dict of dict
.
In the description, details about the keys and values can be added, such as keys are addresses, and values are dictionaries in which the keys are coin types and the values are balances ...
Just as a rough example.
This is good to merge. The app works ( /cc @amiller |
Implement the volume matching algorithm and test cases for it.