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

check allocation based on all projects #387

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

lovelgeorge99
Copy link
Collaborator

No description provided.

Copy link

vercel bot commented Dec 17, 2024

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

Name Status Preview Comments Updated (UTC)
q-acc ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 20, 2024 3:23pm
q-acc-early-access ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 20, 2024 3:23pm

Copy link
Contributor

@aminlatifi aminlatifi left a comment

Choose a reason for hiding this comment

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

@lovelgeorge99 please coordinate with @bens-magic about the policy you have chosen, checking projects one by one to be sure the round is done

const results = [];
for (const project of allProjects) {
const result = await getTokenPriceRangeStatus({ allRounds, project });
results.push(result.isPriceUpToDate);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can exit by seeing the first false.

Comment on lines 47 to 62
const { data: allRounds } = useFetchAllRound();
const [isAllocationDone, setIsAllocationDone] = useState(false);

useEffect(() => {
const cheakAllProjectStatus = async () => {
if (allProjects && allRounds) {
const allProjectsReady = await checkAllProjectsStatus(
allProjects?.projects,
allRounds,
);
setIsAllocationDone(allProjectsReady);
}
};

cheakAllProjectStatus();
}, [allProjects, allRounds]);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not efficient, it will load per project card. We can calculate it once in the useFetchAllRound

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the useFetchAllRound only provides if the isBatchMintingExecuted or not ..we also need to check if the price of the tokens is up to date for the each project which is done here src/services/tokenPrice.ts/getTokenPriceRangeStatus that is why i used this

Copy link
Contributor

Choose a reason for hiding this comment

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

We may find another place. The point is that logic is repeated twice in code, and in one of them (if I am not mistaken) will be executed in each card!
It's better move it to a hook with useQuery and cache it!

) => {
return useQuery({
enabled: !!allProjects && !!allRounds,
queryKey: ['projectPriceStatus', allProjects, allRounds],
Copy link
Contributor

Choose a reason for hiding this comment

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

allProjects and allRounds types are not of string type, right? If so, having them in the cache key is inefficient! It will cause the query to re-execute again and again.
Maybe the projectPriceStatus is enough since all the projects and all the rounds would not change.

Comment on lines 11 to 21
const [isAllocationDone, setIsAllocationDone] = useState(false);

const { data: projectPriceUpdated } = useCheckProjectPriceStatus(
allProjects,
allRounds,
);

useEffect(() => {
console.log('IsAllocation Done', projectPriceUpdated);
setIsAllocationDone(!!projectPriceUpdated);
}, [projectPriceUpdated]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that useEffect doesn't improve efficiency. As I know, the component will rerender already due to projectPriceUpdated update.

Suggested change
const [isAllocationDone, setIsAllocationDone] = useState(false);
const { data: projectPriceUpdated } = useCheckProjectPriceStatus(
allProjects,
allRounds,
);
useEffect(() => {
console.log('IsAllocation Done', projectPriceUpdated);
setIsAllocationDone(!!projectPriceUpdated);
}, [projectPriceUpdated]);
const { data: projectPriceUpdated } = useCheckProjectPriceStatus(
allProjects,
allRounds,
);
const isAllocationDone = !!projectPriceUpdated;

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok thank you @aminlatifi ...really helpful

Comment on lines 47 to 56
const [isAllocationDone, setIsAllocationDone] = useState(false);

const { data: projectPriceUpdated } = useCheckProjectPriceStatus(
allProjects,
allRounds,
);

useEffect(() => {
setIsAllocationDone(!!projectPriceUpdated);
}, [projectPriceUpdated]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise

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.

2 participants