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: index proposals #19

Merged
merged 3 commits into from
Jul 13, 2023
Merged

feat: index proposals #19

merged 3 commits into from
Jul 13, 2023

Conversation

wgwz
Copy link
Contributor

@wgwz wgwz commented Jul 5, 2023

Closes: #15

@wgwz wgwz requested review from blushi and ryanchristo July 5, 2023 19:18
@wgwz wgwz force-pushed the kyle/15-index-proposals branch from 7614906 to 70b21af Compare July 5, 2023 19:21
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. How are you testing this? One thing I never got around to with my prototype was testing to make sure the final status of the proposal is stored correctly, which is probably updated before being pruned but there might be some edge cases.

Comment on lines 44 to 45
normalize["status"],
normalize["tally_result"],
Copy link
Member

Choose a reason for hiding this comment

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

We can limit space and remove these two items. They are included in the fetch_proposal response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What fields do we think would be best to be flat here?
I.e. for the retirements we went through what fields need to be presented in the UI, and gave each field it's own column in the retirements table
I think we should aim for that here too so the client doesn't have to parse json
Do we have a UX design related to how the frontend displays the historical proposals?
That could give us some hints about which fields to pull out of the json into dedicated columns

Copy link
Member

@ryanchristo ryanchristo Jul 5, 2023

Choose a reason for hiding this comment

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

I think the way we use json is actually a positive in that the state of proposal is not really something we define but rather what exists on chain. If the properties of a proposal are updated on chain, we simply store the new properties. This would also ensure support for multiple chains with different versions.

The frontend is currently expecting an array of proposals in json format that match the state of the proposals on chain. I think it might make sense to continue replicating the existing endpoint, maybe not. We would need to write a graphql query that fetches proposals by group id and group policy. If we need additional fields for queries outside the json object, maybe those that we want to look up proposals by field (i.e. group id and policy address).

Maybe @blushi can comment on best approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i’m presuming we probably have certain fields that we want to index (in the database sense of the word). either the client has to parse the json or we can parse it here, i’ve been leaning towards making the consumption of this “indexed” data (in our sense of the word) as straightforward as possible, which is why in general i think it’s best to avoid delivering json to the client from a graphql query. i consider it a layer where we can hide away certain implementation details. but maybe it’s fine 🤷

Copy link
Member

@blushi blushi Jul 10, 2023

Choose a reason for hiding this comment

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

I believe being able to query by policy address is sufficient, given the current implementation on the front-end: https://github.com/regen-network/groups-ui/blob/bcd6730566a6928b07ce246444c0e07f2b5a566d/src/hooks/use-query.ts#L80C17-L80C17

Here's the interface that the UI is expecting for proposals:
https://github.com/regen-network/groups-ui/blob/bcd6730566a6928b07ce246444c0e07f2b5a566d/src/types/proposal.types.ts#L22
This might also be helpful: https://github.com/regen-network/groups-ui/blob/bcd6730566a6928b07ce246444c0e07f2b5a566d/src/api/proposal.utils.ts#L65

I think it makes sense to try to replicate what we have in ProposalSDKType (snake case) / Proposal (camel case version) interfaces from @regen-network/api

So I don't think we should put the result from fetch_proposal in some metadata column for this reason but also because there's already an on-chain proposal metadata field, which could be quite confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense re. following those interfaces and not using the metadata column as we've done here.
so if we follow those interfaces, for example, we have to add the final_tally_result? field, which has this interface:

export interface TallyResultSDKType {
  yes_count: string;
  abstain_count: string;
  no_count: string;
  no_with_veto_count: string;
}

so in terms of the database, do we want to implement final_tally_result as a JSONB column, and we just make sure the JSON going in each row there adheres to the interface above?

and a similar thing would apply for proposers: string[]. we could make use of a JSONB column for that, and ensure we have a list of strings there.

WDYT? am i understanding that correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

never mind, now that i'm looking at this closer i think it's fairly obvious we'll need these to be JSONB and it should pretty straightforward

Copy link
Member

Choose a reason for hiding this comment

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

yeah I think we need jsonb for final_tally_result but proposers could be text[], this way it's directly translated into a string array from the graphql endpoint

@wgwz
Copy link
Contributor Author

wgwz commented Jul 5, 2023

Looking good. How are you testing this? One thing I never got around to with my prototype was testing to make sure the final status of the proposal is stored correctly, which is probably updated before being pruned but there might be some edge cases.

@ryanchristo I'm just running a version of the script you had written for submitting group proposals. I basically adapted that in small ways for my local setup. But it's doing the exact same thing. At the end of the day all I really cared about for testing this was that in my local setup I was getting the EventProposalPruned etc.. After detecting these events in the indexer, I just issue the API call to find the state of the proposal in the block prior to when the "pruning" occurred. Are you talking about the edge case that Cory mentioned a while ago?

Worth noting there is an edge case that may be tricky to nail down with this approach. Proposals that are submitted with —try exec where a member’s vote is sufficient to pass a proposal might not actually ever have the proposal in state at any height at all.
It’s Debatable if this use case is even something worth tracking as a “proposal” in the UI. But we should at least track the edge case in GitHub it if we do move fwd with the implementation you outline here.
ref

If so, I think we can just open an issue for that and put it through prioritization rather than fixing it here. But if you're thinking of something else, LMK!

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.

Did we decide to leave off the votes for now?
In the groups-ui app, we expect to be able to show the votes for a given proposal

@wgwz
Copy link
Contributor Author

wgwz commented Jul 10, 2023

Did we decide to leave off the votes for now? In the groups-ui app, we expect to be able to show the votes for a given proposal

@blushi i just haven't thought about the votes part of this yet. so if we're going the route of adhering to the interfaces you shared, i guess that the votes info shows up in final_tally_result. we'll just have to make sure that data is delivered in the correct format. i'll give this a try

@blushi
Copy link
Member

blushi commented Jul 11, 2023

Did we decide to leave off the votes for now? In the groups-ui app, we expect to be able to show the votes for a given proposal

@blushi i just haven't thought about the votes part of this yet. so if we're going the route of adhering to the interfaces you shared, i guess that the votes info shows up in final_tally_result. we'll just have to make sure that data is delivered in the correct format. i'll give this a try

The thing is that the UI expects more info than just the final tally result, see screenshot below:

image

So at this point either we simplify the UI as part of the MVP so we only show the final tally result (number of yes/no/etc. votes) for historical proposals or we need to also index votes.
By the way, currently, we display the current weight of the voter, but the weight could have been different at the time of the vote (in case of group member update after the vote), so the current implementation is a bit flawed... Because I guess we would want to display the weight of the voter at the time of the vote instead on the proposal page. cc/ @erikalogie @ryanchristo

@wgwz
Copy link
Contributor Author

wgwz commented Jul 11, 2023

The thing is that the UI expects more info than just the final tally result, see screenshot below:

image

So at this point either we simplify the UI as part of the MVP so we only show the final tally result (number of yes/no/etc. votes) for historical proposals or we need to also index votes. By the way, currently, we display the current weight of the voter, but the weight could have been different at the time of the vote (in case of group member update after the vote), so the current implementation is a bit flawed... Because I guess we would want to display the weight of the voter at the time of the vote instead on the proposal page. cc/ @erikalogie @ryanchristo

I see so I guess this would mean that we need to know the block height at the time the voter cast their vote, so we could then look up the weight at that block height (AKA time). Is there a way we can find out that block height without indexing votes? If so, I'm thinking we could use x-cosmos-block-height header in a REST API call to look up the weight using one of these endpoints:

Screen Shot 2023-07-11 at 11 35 07 AM

@blushi
Copy link
Member

blushi commented Jul 11, 2023

So in this case i think we would need to look at the https://buf.build/cosmos/cosmos-sdk/docs/main:cosmos.group.v1#cosmos.group.v1.EventVote events, get the vote information as well as the voter weight at that height and then index the vote (noting that a vote can be changed)
But let's first see with @erikalogie whether we can skip this part for now and just display the final tally result on the proposal page instead.

@wgwz
Copy link
Contributor Author

wgwz commented Jul 11, 2023

So in this case i think we would need to look at the https://buf.build/cosmos/cosmos-sdk/docs/main:cosmos.group.v1#cosmos.group.v1.EventVote events, get the vote information as well as the voter weight at that height and then index the vote (noting that a vote can be changed) But let's first see with @erikalogie whether we can skip this part for now and just display the final tally result on the proposal page instead.

@blushi i spoke with @ryanchristo about this some more today, and we agreed that it's probably best to track this in separate issue. i think we can probably wrap up a version of this proposals indexing work that doesn't have the votes part in it initially.

@blushi
Copy link
Member

blushi commented Jul 12, 2023

So in this case i think we would need to look at the https://buf.build/cosmos/cosmos-sdk/docs/main:cosmos.group.v1#cosmos.group.v1.EventVote events, get the vote information as well as the voter weight at that height and then index the vote (noting that a vote can be changed) But let's first see with @erikalogie whether we can skip this part for now and just display the final tally result on the proposal page instead.

@blushi i spoke with @ryanchristo about this some more today, and we agreed that it's probably best to track this in separate issue. i think we can probably wrap up a version of this proposals indexing work that doesn't have the votes part in it initially.

Sounds good to me too!

@wgwz wgwz force-pushed the kyle/15-index-proposals branch from 2a652d9 to 42a57ad Compare July 12, 2023 20:53
@wgwz wgwz requested review from blushi and ryanchristo July 12, 2023 20:55
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 using #23. I'm sure we could do more thorough tests and monitor in staging but this looks good to me as a starting point.

The logs notified me when the proposal was being indexed:

indexer_container | 2023-07-13 01:25:04,616 - index_blocks - INFO - indexing regen-local block 25
indexer_container | 2023-07-13 01:25:04,634 - index_blocks - INFO - height=25 len(txs)=1
indexer_container | 2023-07-13 01:25:04,817 - index_proposals - INFO - proposal inserted...

And the proposal was successfully stored in the database:

                type                 | block_height | tx_idx | msg_idx | chain_num |       timestamp        |                             tx_hash                              | proposal_id |          status          |                       group_policy_address                       | metadata |                   proposers                    |          submit_time          | group_version | group_policy_version |                                  final_tally_result                                  |       voting_period_end       |         executor_result          | messages 
-------------------------------------+--------------+--------+---------+-----------+------------------------+------------------------------------------------------------------+-------------+--------------------------+------------------------------------------------------------------+----------+------------------------------------------------+-------------------------------+---------------+----------------------+--------------------------------------------------------------------------------------+-------------------------------+----------------------------------+----------
 cosmos.group.v1.EventProposalPruned |           25 |      0 |       0 |         1 | 2023-07-13 01:24:58+00 | e07c31be6fb6324bd11268de8be19922038a0eb2943135f090592e761bb0febc |           1 | PROPOSAL_STATUS_ACCEPTED | regen1afk9zr2hn2jsac63h4hm60vl9z3e5u69gndzf7c99cqge3vzwjzs475lmr |          | {regen1l2pwmzk96ftmmt5egpjulyqtneygmmzndf7csk} | 2023-07-13 01:24:28.817902+00 |             1 |                    1 | {"no_count": "0", "yes_count": "1", "abstain_count": "0", "no_with_veto_count": "0"} | 2023-07-13 01:24:48.817902+00 | PROPOSAL_EXECUTOR_RESULT_NOT_RUN | []

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.

utACK

@wgwz wgwz merged commit 42a57ad into main Jul 13, 2023
@wgwz wgwz deleted the kyle/15-index-proposals branch July 13, 2023 14:32
@wgwz wgwz temporarily deployed to regen-analytics July 13, 2023 14:32 Inactive
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.

Create a process for indexing group proposals
3 participants