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

Update topic schema + add migration for weighted topics #9153

Merged
merged 11 commits into from
Sep 12, 2024

Conversation

rbennettcw
Copy link
Contributor

@rbennettcw rbennettcw commented Sep 9, 2024

Link to Issue

Closes: #9132

Description of Changes

  • Updates Topics schema to support weighted voting per topic
  • Adds migration to update Topics table + apply 1-to-1 constraint for Contest to Topic
  • Updates UI to restrict to 1 topic selection per contest

Test Plan

  • Create multiple contests, each assigned to multiple topics
  • Ensure that the migration runs and each contest is afterwards only associated with 1 topic

Deployment Plan

N/A

Other Considerations

  • There's data loss due to some contests going from multiple topics to a single topic. But the end-user impact will likely be minimal. At worst, any ongoing contests will only be active within a single topic.
  • Technically, we should probably remove the ContestTopics table, but the surface area of that change would be quite large + the spec could change at any time.

Copy link
Collaborator

@timolegros timolegros left a comment

Choose a reason for hiding this comment

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

Changes look good but I can't test because every transition I initiate a transaction I get this error (on master as well):
image

@rbennettcw
Copy link
Contributor Author

Changes look good but I can't test because every transition I initiate a transaction I get this error (on master as well): image

I had the same issue as well but resolved it. Try pulling the latest (I just merged latest master), then logging out then logging back in.

Copy link
Collaborator

@timolegros timolegros left a comment

Choose a reason for hiding this comment

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

I feel like I'm missing some context here. The UI is still allowing me to select multiple topics when creating a contest which leads to this API error:

[08:28:59.887] WARN (utils.ts): BAD_REQUEST: [ZodError] contest.createContestMetadata: [
  {
    "code": "too_big",
    "maximum": 1,
    "type": "array",
    "inclusive": true,
    "exact": false,
    "message": "Array must contain at most 1 element(s)",
    "path": [
      "topic_ids"
    ]
  }
]

Why are we restricting contests to a single topic? Is that a necessary change to decouple stake from contests? I need some more context to understand what the proper functionality should be here.

@masvelio
Copy link
Contributor

The UI is still allowing me to select multiple topics when creating a contest which leads to this API error:

I think in this case we should change it to allow selecting only one topic + removing "all". Also toggles for single option is not best solution. Probably we should go with CWSelectList in this case. @rbennettcw let me know if I can help out

@rbennettcw
Copy link
Contributor Author

rbennettcw commented Sep 12, 2024

@timolegros Voting weight is now dependent on the token configuration of the associated topic (stake or ERC20). If there are multiple topics associated, then we won't know which token config to use for calculating voting weight. It was a decision made by product to restrict to 1 topic/token per contest regarding voting weight.

@masvelio @timolegros I've already updated the UI to restrict to a single topic and remove the All button. See the last commit. Please pull the latest.

Copy link
Contributor

@masvelio masvelio left a comment

Choose a reason for hiding this comment

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

clicked around, works fine

@Rotorsoft
Copy link
Contributor

Wondering if we really need contest_topic now that the association is 0 or 1 at the topic level? We created this table to support multiple topics per contest, but if we need to enforce 1-1 we can simplify by. moving contest_id to topic as optional. Am I missing something?

@rbennettcw
Copy link
Contributor Author

Wondering if we really need contest_topic now that the association is 0 or 1 at the topic level? We created this table to support multiple topics per contest, but if we need to enforce 1-1 we can simplify by. moving contest_id to topic as optional. Am I missing something?

My concerns are:

  • The spec is going through ongoing changes. I think that maintaining the ability to easily switch back to a 1-to-many relationship is not a bad idea, just in case Product changes their mind and finds a way to make it work.
  • Removing the table requires updating queries in the Contest Worker policy, which then requires more extensive testing in that area. If the table is unchanged, then there's no chance of regression with the CWP.

@Rotorsoft
Copy link
Contributor

Wondering if we really need contest_topic now that the association is 0 or 1 at the topic level? We created this table to support multiple topics per contest, but if we need to enforce 1-1 we can simplify by. moving contest_id to topic as optional. Am I missing something?

My concerns are:

  • The spec is going through ongoing changes. I think that maintaining the ability to easily switch back to a 1-to-many relationship is not a bad idea, just in case Product changes their mind and finds a way to make it work.
  • Removing the table requires updating queries in the Contest Worker policy, which then requires more extensive testing in that area. If the table is unchanged, then there's no chance of regression with the CWP.

It's all good, as long as we keep an eye on it for future refactoring. This is accidental complexity right now.

@rbennettcw rbennettcw merged commit e14f55e into master Sep 12, 2024
10 checks passed
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.

Update Topic Schema for Weighted Topics
4 participants