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

(govern) display contract config fields on details page #76

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

Tanya-atatakai
Copy link
Collaborator

Proposed changes

Added staking contract config fields on details page in Govern. It's copy-pasted from Launch with some updates in both:

  • useMaxNumServices and useRewardsPerSecond and can be requested with help of useStakingContractConstant, moved it close to the other similar hooks
  • in some fields values we repeated "seconds" or "OLAS" even thought we have it in labels already, removed that; also changed (sec) to , seconds so that they match the structure of other fields
  • Updated / copy-pasted tests

I know it could be somehow a shared component in the libs folder, but the labels might change in the future, because they don't suit for the governor role

image

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Copy link

vercel bot commented Aug 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
bond ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 12, 2024 3:43pm
govern ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 12, 2024 3:43pm
launch ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 12, 2024 3:43pm
registry ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 12, 2024 3:43pm
tokenomics ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 12, 2024 3:43pm

Copy link
Collaborator

@mohandast52 mohandast52 left a comment

Choose a reason for hiding this comment

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

@Tanya-atatakai Sorry, but I wouldn’t copy-paste. Instead, we could create a different feature library and import it—this is one of the key aspects of a monorepo. Maybe feature-staking-contract 🤔 ?

@oaksprout
Copy link
Collaborator

Sorry, but I wouldn’t copy-paste

@mohandast52 makes sense to discuss that but I wouldn't want that to block the feature going live. If you agree to refactor that then let's do it in a follow up PR.

@mohandast52
Copy link
Collaborator

@oaksprout Sure, I’ll approve it. However, I believe it shouldn’t be a major change—just create a library, copy-paste, and import.

@Tanya-atatakai
Copy link
Collaborator Author

@mohandast52 that's why I didn't create a separate lib - I was struggling with where to put it, both common-contract-functions and ui-components didn't look like a good fit. I don't think it makes sense to create an entire library just to store a few files that are similar across some apps. In the end, we risk having too many different lib folders, which could lead to confusion.

Maybe we can discuss this in our next engineering meeting? Generally, for this purpose, we could create something like the shared lib as in nx-examples

@truemiller would like to hear your thoughts too

@Tanya-atatakai Tanya-atatakai requested a review from 77ph August 12, 2024 16:04
@DavidMinarsch DavidMinarsch merged commit a726a5d into main Aug 12, 2024
9 checks passed
@DavidMinarsch DavidMinarsch deleted the tanya/govern-contract-fields branch August 12, 2024 17:15
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.

4 participants