-
Notifications
You must be signed in to change notification settings - Fork 112
Reputation Mining Visualizer, Part 1 #582
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
Conversation
dbb712e to
d261fee
Compare
d5d47ab to
b96e894
Compare
9961f2e to
52ca639
Compare
b1f855a to
1929c8f
Compare
ca4b929 to
e3232a1
Compare
108d7aa to
094df4f
Compare
| <div id="app"> | ||
| <h2>Welome to RepMinCycViz!</h2> | ||
| <h3>Mining Cycle Address:</h3> | ||
| <input placeholder="Mining Cycle Address" v-model="cycleAddress"> |
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.
Will this work across mining cycles? i.e. when the cycle completes and the cycleAddress changes?
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.
Currently you have to manually update the mining cycle address. Over the weekend I was thinking of ways to have the visualizer automatically fetch the address -- doing the fetch on load should be no problem, but automatically refreshing when the cycle updates would be a bit more involved. At minimum refreshing the visualizer should give you the current mining cycle.
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.
In #596 we had a similar requirement and ended up listening for the ReputationMiningCycleComplete event. These 2 PRs are very close in that they improve the quality of life of reputation miners so perhaps we can rebase this on top of #596 and hook into that event handler as well?
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.
Mm I'll look into it. It may not be necessary though, since unlike the miner which is meant to be running automatically in the background, users will generally be interacting with the visualizer manually, so a manual refresh (especially given that mining cycles are an hour+) doesn't seem too bad. Although I could imagine someone potentially wanting to leave the visualizer running on a screen for a longer period of time, in which case the automatic update would be useful.
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.
Although I'd still say that we should merge this as-is and we can add the auto-update functionality in a later PR.
| @@ -0,0 +1,99 @@ | |||
| /* globals artifacts */ | |||
| const { WAD, MINING_CYCLE_DURATION } = require("../../../helpers/constants"); | |||
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.
The setup here is in fact only hydrating some test data in the current cycle. I don't think it's necessary except for testing. In practice the visualiser should probably be a part of the ReputationMiningClient and started in its constructor or initialise call.
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.
You're right, it's only for testing. I can move it elsewhere to make that more clear.
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.
Maybe it fits in the test-data-generator.js? Or we don't need it at all? For development purposes we can also hook into the now live Goerli deployment where there are already some test log entries to process.
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.
I'd say leave it for now, since having more precise control over the test data will be useful for at least a while longer. I'd say leave it in scripts since truffle scripts run in a different environment than the test code.
e3db835 to
bb5fa49
Compare
elenadimitrova
left a comment
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.
Tested against the current Goerli reputation state and looks lovely:
Really good work on this @kronosapiens 👏
| parseLogEntry: function (logEntry) { | ||
| return { | ||
| user: logEntry.user.slice(0, 8), | ||
| amount: ethers.utils.formatEther(logEntry.amount._hex), |
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 formatEther gives me back for a value of 10 -> "amount": "0.00000000000000001"` which I find counter intuitive. Also we're not talking about Ether value here but reputation or token amounts which might not be 18 decimals. My preference for now would be to show the actual value but open to other suggestions.
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.
Fair
| }, | ||
| parseLogEntry: function (logEntry) { | ||
| return { | ||
| user: logEntry.user.slice(0, 8), |
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.
Please please don't slice the user! Seeing "user": "0xb77D57" is really unhelpful
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.
Fair, I did that at the beginning so the entries would easily render on one line but it's not a great production feature.
| user: logEntry.user.slice(0, 8), | ||
| amount: ethers.utils.formatEther(logEntry.amount._hex), | ||
| skillId: parseInt(logEntry.skillId._hex), | ||
| colony: logEntry.colony.slice(0, 8), |
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.
What's with the slicing of addresses?
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.
Same as above, I wanted things to render once-per-line for development.
| <option>Inactive Cycle</option> | ||
| </select> | ||
|
|
||
| <p>Cycle Address: {{ cycleAddress }} </p> |
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.
Can we add the open timestamp and maybe remaining time to close here? I was needing this information this morning to determine where my updates from 10am have gone.
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.
Sure!
| miningCycle: {}, | ||
| numEntries: 0, | ||
| logEntries: [], | ||
| numSubmittedHashes: 0, |
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.
Can we rename this to numUniqueSubmittedHashes everywhere including the way it displays Num Submitted Hashes -> Num Unique Submitted Hashes. We renamed this in the last few days to differentiate better between the number of submissions of a particular hash (up to 12) and the number of unique hashes.
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.
Sure!
| <div id="app"> | ||
| <h2>Welome to RepMinCycViz!</h2> | ||
| <select v-model="selected"> | ||
| <option>Active Cycle</option> |
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.
There is a problem with the initial load, although the "Active cycle" appears selected it is not loaded in the visualiser. I don't particularly mind refreshing the dropdown but if we can auto-load this the first time it'll be better.
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.
I couldn't immediately think of a clean way to do that in Vue, but I can think harder.
| <html lang="en"> | ||
| <head> | ||
| <meta charset="utf-8"> | ||
| <title>Colony Mining Cycle Visualizer</title> |
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.
Can we have a non-comic shrift font for this please :)
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.
a l r i g h t
| @@ -0,0 +1,320 @@ | |||
| <!-- Forked from: https://bl.ocks.org/d3noob/43a860bc0024792f8803bba8ca0d5ecd --> | |||
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.
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.
Yup!
| <!-- Page HTML --> | ||
| <div id="app"> | ||
| <h2>Welome to RepMinCycViz!</h2> | ||
| <select v-model="selected"> |
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.
Can we add the current reputation root hash here please
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.
Sure!
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.
Is that from the ColonyNetwork contract or the ReputationMiningCycle? If the former, it would require bringing the ColonyNetwork ABI into the visualizer which I'd like to avoid if possible since it's more data and complexity (although doable if need be).
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.
It would need to come from ColonyNetwork
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.
What's the benefit of being able to see the root hash itself?
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.
The most obvious benefit I can see other than just being nice to see is that you would then have all the information needed to query the oracle to get the merkle proofs (maybe the reputation values could even link to the relevant page on the local oracle?)
| @@ -0,0 +1,320 @@ | |||
| <!-- Forked from: https://bl.ocks.org/d3noob/43a860bc0024792f8803bba8ca0d5ecd --> | |||
| <!DOCTYPE html> | |||
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.
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.
Will investigate.
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.
Ok should be good now.
75da52e to
6707e0e
Compare
7b4a736 to
2e0e6e0
Compare
|
Some open todos:
|
elenadimitrova
left a comment
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.
Just a couple of remaining bits and we can merge
docs/_Docs_Releases.md
Outdated
| --- | ||
|
|
||
|  | ||
|  |
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 already part of #614
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.
Ok, I'll remove.
| "posttest:contracts:watch": "npm run stop:blockchain:client", | ||
| "posttest:contracts:security": "sed -ie \"s/docker: false/docker: true/g\" ./truffle.js && rimraf ./truffle.jse" | ||
| "posttest:contracts:security": "sed -ie \"s/docker: false/docker: true/g\" ./truffle.js && rimraf ./truffle.jse", | ||
| "viz:bootstrap": "npm run start:blockchain:client & truffle migrate --reset --compile-all && truffle exec --network development scripts/viz-bootstrap.js" |
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.
Did we not decide the bootstrapping was unnecessary?
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.
No, we still disagree. I'd like to leave it in for now, we can remove it once the visualizer is more mature.
| @@ -0,0 +1,15 @@ | |||
| # Welcome to the Colony Mining visualizers! | |||
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.
I'd argue we remove this readme and incorporate information on running the visualiser in the network docs for the miner. Otherwise this feels too separate from the client and I'd prefer to think of them as the UI and logic to the same thing.
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.
Ok, I can move the readme.
2e0e6e0 to
f17b605
Compare
f17b605 to
8552dd7
Compare







Closes #550 (but not the end of road)
Changes Summary
This PR includes two
.htmlfiles which implement self-contained visualizers for reputation state (no transpiling / packing required!), served by the reputation miner (as well as anindex.htmlfor the mining client)The first,
repCycle.html, supports viewing of a ReputationMiningCycle contract, specifically the log entries and dispute state.The second,
repTree.htmlsupports viewing of the reputation tree accessed via a mining client.Neither is complete yet, lots of room to grow, but both do something so let's get this party started!
Setup Instructions
To visualize data from an existing network, simply spin up a mining client which points to the network in question. To generate test data on a local ganache network, run
yarn run viz:bootstrapand then paste the resulting command into a new terminal.Once the miner is running, navigate to
http://localhost:3000/.Alternatively, spin up a mining client and point it to the Görli testnet to view live testnet data.
Screenshots
Home Page
Rep Cycle Explorer
Rep Tree Visualizer