-
Notifications
You must be signed in to change notification settings - Fork 128
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
[gov] Migrate to new politeia tlog API #1829
Conversation
This is ready for a review @chappjc and @buck54321 if also interested mate. Need to delete |
Thanks, @thi4go . I got it. |
Going to review this today after I push a couple fixes. Don't worry about the conflicts for now @thi4go |
Sounds good mate @chappjc 👍 |
I'm started reviewing, but the merge commit from master makes squashing it a little trickier than usual. Please prefer rebase to merge in the future. But even with numerous commits, it's often easier to squash first without changing the base commit and then rebase rather than doing it in one step with What I'm reviewing now is this PR squashed, then rebased on master. I chose to do it this time like the following (after pulling latest master):
but could also have done:
This is the single rebased commit I came up with: chappjc@6e5d940 |
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 had about 30 other line comments that github just discarded. Here's one and I'll write my review again later.
Hey mate @chappjc thanks for the review. This is ready for another look, comments addressed. |
Great. Apologies for failing to alert you to the axios elimination. |
Hey @chappjc thx again for the review. Second round of comments addressed 👍 |
This was to isolate the storm dependency too. |
On the Daylight Robery prop that's voting right now, I'm getting the following at http://127.0.0.1:7777/proposal/ae609f183a031c34: Seeing the following on the Dubai prop that's not voting yet (http://127.0.0.1:7777/proposal/51c412822e5e4b59), but also on the Storyteller prop: Seeing this on an accepted prop like Automatic Ticket Revocations (http://127.0.0.1:7777/proposal/e2d7b7d6b837431f), same as the ones that are currently voting: I've rebuilt everything a couple times. |
The charts broke after I ditched axios, should have changed the way we get the response, my bad mate @chappjc . Pushed a fix for it. The |
Ahhh, yes, having removed a ton of |
I'm just hacking wildly around Stimulus now, but I got around the proposal.token error with the following: diff --git a/cmd/dcrdata/public/js/controllers/proposal_controller.js b/cmd/dcrdata/public/js/controllers/proposal_controller.js
index b27fb022..a24cc85f 100644
--- a/cmd/dcrdata/public/js/controllers/proposal_controller.js
+++ b/cmd/dcrdata/public/js/controllers/proposal_controller.js
@@ -118,20 +118,20 @@ export default class extends Controller {
}
async connect () {
- if (this.hasApprovalMeterTarget) {
- const d = this.approvalMeterTarget.dataset
- const opts = {
- darkMode: darkEnabled(),
- segments: [
- { end: d.threshold, color: '#ed6d47' },
- { end: 1, color: '#2dd8a3' }
- ]
- }
- this.approvalMeter = new MiniMeter(this.approvalMeterTarget, opts)
+ if (!this.hasApprovalMeterTarget) return // there will be no meter or charts
+
+ const d = this.approvalMeterTarget.dataset
+ const opts = {
+ darkMode: darkEnabled(),
+ segments: [
+ { end: d.threshold, color: '#ed6d47' },
+ { end: 1, color: '#2dd8a3' }
+ ]
}
+ this.approvalMeter = new MiniMeter(this.approvalMeterTarget, opts)
chartData = await requestJSON('/api/proposal/' + this.tokenTarget.dataset.hash)
-
+ if (!chartData) return
Dygraph = await getDefault(
import(/* webpackChunkName: "dygraphs" */ '../vendor/dygraphs.min.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.
Looks good now, and working well.
Some follow-up for me:
- the stimulus fix described in my last comment for when there are no votes and thus no approval meter or charts
- DB upgrade to drop the orphaned proposals table
Somehow snapshotting the old props (vote data extracted from git repo), and importing all that into the new@thi4go is working on loading the legacy json files from the politeiagui repo into the actual tlog so we will eventually get the legacy prop info for freeProposalsDB
Nevermind, just learned about |
This diff implements the changes necessary to migrate to the new politeia tlog APIs. All data is synced through politeia APIs and stored on stormdb for now. Talked to @chappjc and we might want to drop the stormdb dependency in the near future, so this data might go to a pg table instead of being stored on storm. Major changes on this PR were:
go-piparser
that dcrdata needed as part of the logic of fetching the git repos and parsing votes data.proposals
andproposal_votes
are no longer used anywhere in the code.I think there isn't a need for the gov package to be a standalone module anymore. I left it be, but if you agree on this, I can make it part of this PR.
About supporting the legacy proposals, politeiagui is using legacyproposals.json and legacyvotestatuses.json to display part of the data. These would be enough for the dcrdata UI if we import them, except for the tickets vote data for the charts. So I think we have two options:
proposals
andproposal_votes
tables.With this migration we lose the ability to reconstruct the pg tables if we ever need it, since we dropped the
go-piparser
dep and all the legacy code behind it, so I'm not sure how I feel about the first option. The data will always be availabe on git if someone ever wants the parse it themselves and verify it, so I tend to think it's okay not showing charts for the legacy props.Adding support for legacy proposals will be done next in a fresh PR in order to facilitate review.
closes #1816 and #1823