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: Add checks against contract no funds and full slots cases #162

Merged
merged 29 commits into from
Jul 2, 2024

Conversation

mohandast52
Copy link
Collaborator

@mohandast52 mohandast52 commented May 31, 2024

  • The user should not be able to start the agent if there are no rewards available or if all slots are full.
  • Please check the conditions here
j k

@mohandast52 mohandast52 added the enhancement New feature or request label May 31, 2024
@mohandast52 mohandast52 self-assigned this May 31, 2024
@mohandast52 mohandast52 marked this pull request as ready for review May 31, 2024 14:16
@mohandast52
Copy link
Collaborator Author

@oaksprout please suggest on message copies please 🙏

@truemiller
Copy link
Collaborator

truemiller commented May 31, 2024

would usually moan about new dependencies, but i think zustand looks really cool, just skimmed through it's docs, will get up to date with the package over weekend

on second thoughts, we should not do this :/
#162 (comment)

@oaksprout
Copy link
Collaborator

Maybe this could be simpler:

What if the button text became "Cannot start agent ℹ️"

And then a tooltip shows when you hover over which lists the reasons why:

- No rewards available
- No jobs available

"Jobs" being a human-friendly concept for 'slots'.

Curious to hear Roman's thoughts.


enum ServiceButtonLoadingState {
Starting,
Pausing,
NotLoading,
}

const FirstRunModal = ({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved to a separate file - FirstRunModal.tsx

Copy link
Collaborator

@truemiller truemiller Jun 27, 2024

Choose a reason for hiding this comment

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

either move this to hooks folder (as it exports a hook)
or move the store folder to the root, it does not contain components

Copy link
Collaborator

Choose a reason for hiding this comment

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

the file name has typo too :/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • moved to the root folder
  • but I still believe we should keep related components/hooks/functions together. In this case, it should be in MainHeader since no other component uses it. We can move it if other components need it in the future instead of putting everything in the hooks or functions folder. Otherwise, it will eventually become cluttered and difficult to manage.

@truemiller truemiller self-requested a review June 27, 2024 16:38
Copy link
Collaborator

@truemiller truemiller left a comment

Choose a reason for hiding this comment

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

not sure if the above changes were added recently or not, but really need to stop putting things in random places

Copy link
Collaborator

@Tanya-atatakai Tanya-atatakai left a comment

Choose a reason for hiding this comment

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

1 tiny comment, lgtm overall

@truemiller
Copy link
Collaborator

truemiller commented Jun 30, 2024

@mohandast52 i'm sorry, but on thinking about this more, we should not include zustand.
it's a great library, but we should use what is in place already. so we can avoid situations where, for example, we have a mix of web3, ethers, viem .etc 🙏

@mohandast52
Copy link
Collaborator Author

but we should use what is in place already

@joshmlxn, the current implementation is making the provider more complex. I suggest we include zustand and gradually migrate the context to zustand. We might not get a dedicated time for refactoring, so incorporating such libraries now and transitioning gradually would be more practical.

Screenshot 2024-07-01 at 3 57 35 PM

const [canStartAgent, setCanStartAgent] = useState(false);

useEffect(() => {
(async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we'll need to extract this into a useCallback function at a later point so we can call it on demand, and also update the state via interval. but it's fine for now 👍

@mohandast52 mohandast52 merged commit 2bcbc0c into main Jul 2, 2024
4 checks passed
@mohandast52 mohandast52 deleted the mohan/checks-against-contract-no-funds branch July 2, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants