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: historical proposals #112

Merged
merged 33 commits into from
Aug 3, 2023
Merged

Conversation

wgwz
Copy link
Contributor

@wgwz wgwz commented Jul 20, 2023

Closes: #63

This approach adds support for historical proposals.
Historical proposals are proposals that have been pruned from state.
This approach utilizes graphql queries to our indexer graphql API.
The indexer database is the source of data for historical proposals.

@netlify
Copy link

netlify bot commented Jul 20, 2023

Deploy Preview for regen-groups-ui ready!

Name Link
🔨 Latest commit 0da1360
🔍 Latest deploy log https://app.netlify.com/sites/regen-groups-ui/deploys/64cbfaca3aec3a000854ad3c
😎 Deploy Preview https://deploy-preview-112--regen-groups-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@wgwz wgwz changed the title feat: historical proposals (react-query and graphql-request, no apollo) feat: historical proposals Jul 20, 2023
Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

Looking good. Following up on your question this morning, as far as I can tell, no need to add a provider when using graphql-request client. Let me know how you're ready for a review, currently receiving the following error with my local setup:

Cannot query field "allProposals" on type "Query".: {"response":{"errors":[{"message":"Cannot query field \"allProposals\" on type \"Query\".","locations":[{"line":2,"column":3}]},{"message":"Unknown type \"Proposal\".","locations":[{"line":9,"column":26}]}],"status":400,"headers":{"map":{"content-length":"195","content-type":"application/json; charset=utf-8"}}},"request":{"query":"query ProposalsByGroupPolicyAddress($groupPolicyAddress: String!) {\n allProposals(condition: {groupPolicyAddress: $groupPolicyAddress}) {\n nodes {\n ...ProposalItem\n }\n }\n}\n\nfragment ProposalItem on Proposal {\n type\n blockHeight\n txIdx\n msgIdx\n chainNum\n timestamp\n txHash\n id: proposalId\n status\n groupPolicyAddress\n groupPolicyVersion\n metadata\n proposers\n submitTime\n groupVersion\n groupPolicyAddress\n finalTallyResult\n votingPeriodEnd\n executorResult\n messages\n}","variables":{"groupPolicyAddress":"regen1afk9zr2hn2jsac63h4hm60vl9z3e5u69gndzf7c99cqge3vzwjzs475lmr"}}}

@wgwz wgwz force-pushed the kyle/63-graphql-request-react-query branch from 40e0d14 to 57a5108 Compare July 24, 2023 20:20
@wgwz
Copy link
Contributor Author

wgwz commented Jul 24, 2023

Looking good. Following up on your question this morning, as far as I can tell, no need to add a provider when using graphql-request client.

I ended up adding this, i just saw this comment but I think the why a Provider is needed or recommended, is so that the child components all use the same instance of the client. It turned out to not be too much work to add this in.

Let me know how you're ready for a review, currently receiving the following error with my local setup:

Cannot query field "allProposals" on type "Query".: {"response":{"errors":[{"message":"Cannot query field \"allProposals\" on type \"Query\".","locations":[{"line":2,"column":3}]},{"message":"Unknown type \"Proposal\".","locations":[{"line":9,"column":26}]}],"status":400,"headers":{"map":{"content-length":"195","content-type":"application/json; charset=utf-8"}}},"request":{"query":"query ProposalsByGroupPolicyAddress($groupPolicyAddress: String!) {\n allProposals(condition: {groupPolicyAddress: $groupPolicyAddress}) {\n nodes {\n ...ProposalItem\n }\n }\n}\n\nfragment ProposalItem on Proposal {\n type\n blockHeight\n txIdx\n msgIdx\n chainNum\n timestamp\n txHash\n id: proposalId\n status\n groupPolicyAddress\n groupPolicyVersion\n metadata\n proposers\n submitTime\n groupVersion\n groupPolicyAddress\n finalTallyResult\n votingPeriodEnd\n executorResult\n messages\n}","variables":{"groupPolicyAddress":"regen1afk9zr2hn2jsac63h4hm60vl9z3e5u69gndzf7c99cqge3vzwjzs475lmr"}}}

I'd guess that's it probably because you aren't connected to an up-to-date version of the indexer database, but it's little hard to tell without a bit more info on your local setup

@wgwz wgwz force-pushed the kyle/63-graphql-request-react-query branch from aba4d17 to 5512fdd Compare July 25, 2023 01:15
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

As part of this, we should also probably display the final tally result for proposals that have been pruned instead of the ProposalVotesTable as we had discussed

@wgwz wgwz force-pushed the kyle/63-graphql-request-react-query branch from 03764d4 to 77b8189 Compare July 25, 2023 20:12
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

As part of the groups-ui audit and while testing this, I've realized that there was an existing bug but somewhat related to historical proposals/votes. That happens as soon as votes have been pruned but not the proposal (could happen since votes gets pruned at the end of the voting period while proposal at the latest gets pruned at voting period + max exec period). In this case, on the proposal page, you see "No votes" but the VotesGraph at the top shows votes, which is puzzling:

image

This is because the VotesGraph uses either the proposal finalTallyResult or the votes while the VotesTable is based on votes only.
Now that we have a ProposalFinalTallyTable, maybe we could use in place of the empty votes table in this case?
Also fine to tackle this as a separate issue but I believe this should be fixed for the MVP.

src/components/organisms/proposal-summary.tsx Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@wgwz
Copy link
Contributor Author

wgwz commented Jul 27, 2023

As part of the groups-ui audit and while testing this, I've realized that there was an existing bug but somewhat related to historical proposals/votes. That happens as soon as votes have been pruned but not the proposal (could happen since votes gets pruned at the end of the voting period while proposal at the latest gets pruned at voting period + max exec period). In this case, on the proposal page, you see "No votes" but the VotesGraph at the top shows votes, which is puzzling:

This is because the VotesGraph uses either the proposal finalTallyResult or the votes while the VotesTable is based on votes only. Now that we have a ProposalFinalTallyTable, maybe we could use in place of the empty votes table in this case? Also fine to tackle this as a separate issue but I believe this should be fixed for the MVP.

@blushi I don't mind handle this now or in a separate issue. No strong preference, it doesn't seem too complicated.
I spent some time thinking about how we might be able to handle this and I wanted to check the logic.
Currently, in ProposalSummary we have a check called isVotingClosed.
I'm assuming that when that is true, then the votes have been pruned.
So we could pull this variable out a level higher.
isVotingClosed can be checked within the ProposalPage, and passed to ProposalSummary.
Then we could conditionally render the ProposalFinalTallyTable in ProposalPage if isVotingClosed and there are no votes.
But I guess there will always be no votes if isVotingClosed is true, at least until we have historical votes, so that last part may be redundant for now.
Does that sounds right?

@blushi
Copy link
Member

blushi commented Jul 31, 2023

yeah I think that should work @wgwz

@wgwz
Copy link
Contributor Author

wgwz commented Jul 31, 2023

@blushi i added the fix to show the final tally result table if the voting period is closed:

c82b561

Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

Pre-approving from a code standpoint, but getting some CORS issue on the deploy preview

@wgwz wgwz force-pushed the kyle/63-graphql-request-react-query branch from b0609de to f00bec5 Compare August 1, 2023 16:24
@wgwz
Copy link
Contributor Author

wgwz commented Aug 1, 2023

but getting some CORS issue on the deploy preview

@blushi i think this should be fixed now, although it was a little weird for me at first.
it seems like it just took some time for the deploy preview to pick up some env var changes.
but i also did a hard refresh on the preview as well, so you might need to do the same.
i'm not really sure whether it was time, or a caching issue but seems working now.

but to fix the CORS issue i just needed to update VITE_PROXY_URL_REGEN_MAINNET and VITE_PROXY_URL_REGEN_TESTNET in netlify.
they were using *.herokuapp.com URLs that were using the old heroku app name.
now they point at api.regen.network and api-staging.regen.network

@blushi
Copy link
Member

blushi commented Aug 2, 2023

I found out, this is because we store the URL in local storage:
image

so a hard refresh or deleting this in local storage, and that works back again!

@wgwz
Copy link
Contributor Author

wgwz commented Aug 2, 2023

just writing up the issue i mentioned in our check-in today.
currently this PR assumes that there is only one indexer API for a given deployment.
our indexer environments, staging and production, each connect only to one chain, redwood and mainnet.
so each indexer API only returns data for either redwood or mainnet.

the prod groups-ui offers the toggle between networks, i.e. redwood and mainnet.
this PR as it stands, would introduce code that only works for showing historical proposals from mainnet in prod groups-ui.
if we want the historical proposals to also work for redwood in groups-ui prod, then we need to implement this somewhat differently.
we'll need to have two graphql client instances, one for the staging indexer and one for the prod indexer.
and conditionally use one or the other based on which network is selected in the UI.

an interesting question comes up here.
do we want to support historical proposals for the redwood testnet in production?
i would say the answer to that is no.
i would argue that historical proposals are only important in groups-ui prod, for mainnet.
the database for the indexer is large costly, and if we did this, eventually we would double our costs.

@ryanchristo @blushi curious to hear your thoughts!

@ryanchristo
Copy link
Member

do we want to support historical proposals for the redwood testnet in production?

Yes, a user should be able to switch networks in both staging and production with historical votes available in the groups ui in both staging and production as well. Same way the proxy URLs for both are available in both deployments currently.

@ryanchristo
Copy link
Member

i would argue that historical proposals are only important in groups-ui prod, for mainnet.

To properly use the staging environment for testing, an issue might come up that is specific to mainnet and therefore we would be able to resolve the issue before pushed to production. We may want to use the testnet to test a scenario on redwood and a user should be able to interact with the production deployment using redwood and the same features as mainnet.

@ryanchristo
Copy link
Member

the database for the indexer is large costly, and if we did this, eventually we would double our costs.

I think we can worry about storage separately from including support for both endpoints within the groups-ui.

@ryanchristo
Copy link
Member

I opened #131 as a followup. We can continue the discussion there. Not something we need to address in this pr.

Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

tACK. Nice work!

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.

Add support for viewing historical proposals
3 participants