Skip to content

Handle rented node in grace period #3742

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Feb 24, 2025
Merged

Handle rented node in grace period #3742

merged 13 commits into from
Feb 24, 2025

Conversation

samaradel
Copy link
Contributor

@samaradel samaradel commented Dec 15, 2024 •
edited
Loading

Description

Check if the rented node contract is in the grace period when listing the node.

Changes

image

Related Issues

Tested Scenarios

  • Navigate to the Node finder page.
  • Rent a dedicated node.
  • Leave it till it goes to the grace period.
  • Navigate to any solution.
  • Try to deploy an instance on the rented node.

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstrings
  • Screenshots/Video attached (needed for UI changes)

@samaradel
Copy link
Contributor Author

cpd from #3646

@samaradel samaradel marked this pull request as ready for review December 15, 2024 14:34
0oM4R
0oM4R reviewed Dec 16, 2024
View reviewed changes
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

image

#3646 (review)

@samaradel
Copy link
Contributor Author

@0oM4R check it, it already fixed

amiraabouhadid
amiraabouhadid approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@amiraabouhadid amiraabouhadid left a comment

Choose a reason for hiding this comment

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

no console err
image

@samaradel samaradel requested a review from 0oM4R December 17, 2024 09:24
@samaradel samaradel marked this pull request as draft December 17, 2024 09:43
@AhmedHanafy725 AhmedHanafy725 mentioned this pull request Dec 24, 2024
Merged
5 tasks
@samaradel samaradel marked this pull request as ready for review February 2, 2025 21:23
0oM4R
0oM4R reviewed Feb 3, 2025
View reviewed changes
Copy link
Contributor

@0oM4R 0oM4R left a comment •
edited
Loading

Choose a reason for hiding this comment

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

on first if the node is selected;
we won't see any error but if you select another node then click on the graceperiod node will see the error

also the error must be handled

  • the node is not a valid node; so we shouldn't auto select it; need to handle this case as well

image

0oM4R
0oM4R reviewed Feb 3, 2025
View reviewed changes
packages/playground/src/utils/nodeSelector.ts Outdated
Comment on lines 239 to 242
setTimeout(() => {
reject(Error(err));
}, 2000);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think we need to add timeout here, not the auto one there is no toast overlap what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally agree, removed

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you removed timeout in the automatic on to, can you explain why I think ti is a better UX even if we set it to only one sec

@samaradel samaradel marked this pull request as draft February 16, 2025 14:47
@samaradel samaradel marked this pull request as ready for review February 17, 2025 10:43
@samaradel samaradel requested a review from 0oM4R February 17, 2025 11:02
0oM4R
0oM4R requested changes Feb 17, 2025
View reviewed changes
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

graceperiod

error still exsist if i select the grace period node

@samaradel
Copy link
Contributor Author

@0oM4R works with me, please make sure to have the latest update and build

@0oM4R
Copy link
Contributor

0oM4R commented Feb 18, 2025

recurcive.mov

@samaradel samaradel marked this pull request as draft February 18, 2025 12:55
@samaradel samaradel requested a review from 0oM4R February 18, 2025 19:30
@samaradel samaradel marked this pull request as ready for review February 18, 2025 19:30
0oM4R
0oM4R requested changes Feb 20, 2025
View reviewed changes
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

I think we just need the changes in utils only; no need for the other changes

@samaradel samaradel marked this pull request as draft February 20, 2025 17:52
@samaradel samaradel requested a review from 0oM4R February 23, 2025 11:16
@samaradel samaradel marked this pull request as ready for review February 23, 2025 11:16
@samaradel samaradel marked this pull request as draft February 23, 2025 12:07
@samaradel
q
f1c65fe 
- Remove unwanted contract validation
- Early exit for check it node is rented and not in dedicated farm
- Remove the check only if dedicated filter is ON
@samaradel samaradel marked this pull request as ready for review February 23, 2025 13:35
0oM4R
0oM4R approved these changes Feb 23, 2025
View reviewed changes
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

dedicated nodes in shared farms are valid nodes
image

validation works fine on automatic finder

image

all works fine thanks.

image

@samaradel samaradel merged commit 8c936ef into development Feb 24, 2025
10 checks passed
@samaradel samaradel deleted the development_2.7_handle_rentedNode branch February 24, 2025 14:10
@ramezsaeed ramezsaeed mentioned this pull request Feb 24, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramezsaeed ramezsaeed ramezsaeed approved these changes

@amiraabouhadid amiraabouhadid amiraabouhadid approved these changes

@0oM4R 0oM4R 0oM4R approved these changes

@AhmedHanafy725 AhmedHanafy725 Awaiting requested review from AhmedHanafy725 AhmedHanafy725 is a code owner

@zaelgohary zaelgohary Awaiting requested review from zaelgohary zaelgohary is a code owner

@maayarosama maayarosama Awaiting requested review from maayarosama

@MohamedElmdary MohamedElmdary Awaiting requested review from MohamedElmdary MohamedElmdary is a code owner

@Mahmoud-Emad Mahmoud-Emad Awaiting requested review from Mahmoud-Emad Mahmoud-Emad is a code owner

@AlaaElattar AlaaElattar Awaiting requested review from AlaaElattar AlaaElattar is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@samaradel @0oM4R @ramezsaeed @amiraabouhadid