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

Interact with dao contract functions #104

Merged
merged 33 commits into from
Dec 12, 2024
Merged

Conversation

0xExp-po
Copy link
Contributor

@0xExp-po 0xExp-po commented Dec 3, 2024

create a create proposal function

Copy link

netlify bot commented Dec 3, 2024

Deploy Preview for tansu canceled.

Name Link
🔨 Latest commit 0588b41
🔍 Latest deploy log https://app.netlify.com/sites/tansu/deploys/675a2c342b2a8d000863c537

Copy link

netlify bot commented Dec 3, 2024

Deploy Preview for testnet-tansu failed. Why did it fail? →

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/testnet-tansu/deploys/675a3003eb0819b171c8d695

projectName,
proposalName,
directoryCid.toString(),
Date.now() + 86400000 * import.meta.env.PUBLIC_VOTING_PERIOD,
Copy link
Owner

Choose a reason for hiding this comment

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

This needs to be configurable from the user side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it

Copy link
Owner

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Thanks, I left a few suggestions.

I will test it locally now and get back to you.

Copy link
Owner

Choose a reason for hiding this comment

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

We don't need any demo data no? I think you can create all the real proposals from the demo instead. I will try to make one with this branch locally.

Copy link
Owner

Choose a reason for hiding this comment

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

You should not redefine these, everything is defined in the contracts bindings already. The point of this error task was to do some automatic error handling of the contract and show the proper error if the contract fails instead of a number or no error message at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the contract error messages are for the alert to users what contract error occurs.
if we need some other automatic error handling not only error alert, I'll update the response data type which means the service functions only return errorCode, then in the component, according to the errorCode, I'll alert error message and do the other error handling actions.
how do you think about it?

Copy link
Owner

Choose a reason for hiding this comment

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

I meant this, we already define the errors in the bindings here and don't need to duplicate things.

export const Errors = {
0: { message: "UnexpectedError" },
1: { message: "InvalidKey" },
2: { message: "ProjectAlreadyExist" },
3: { message: "UnregisteredMaintainer" },
4: { message: "NoHashFound" },
5: { message: "InvalidDomainError" },
6: { message: "MaintainerNotDomainOwner" },
7: { message: "ProposalInputValidation" },
8: { message: "NoProposalorPageFound" },
9: { message: "AlreadyVoted" },
10: { message: "ProposalVotingTime" },
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, should I alert this messages to the users?

Comment on lines 149 to 157
if (!isDescriptionValid(rejectDescription)) {
alert("Rejected description must contain at least 3 words.");
return;
}

if (!isDescriptionValid(cancelledDescription)) {
alert("Cancelled description must contain at least 3 words.");
return;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Actually it's more complicated. Only for approval it's a must to have a description. For reject and cancelled, description is only required if there is a XDR.

While we are here, if no XDR is provided or no description, then in the UI we don't need to show this specific outcome. I expect that a lot of proposals will just have an approve outcome and the rest will be empty. So we don't need to show empty outcomes.

return;
}

if (votingDays < 5 || votingDays > 30) {
Copy link
Owner

Choose a reason for hiding this comment

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

Make it 1 for the min. 5 is a bit much.

Comment on lines 225 to 229
const directoryCid = await client.uploadDirectory(files);

setIsLoading(false);
alert(
`Proposal submitted successfully! CID: ${getIpfsBasicLink(directoryCid.toString())}`,
const { createProposal } = await import("@service/WriteContractService");

const res = await createProposal(
Copy link
Owner

Choose a reason for hiding this comment

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

I feel that there should be some validation there to prevent calling the smart contract if IPFS upload failed.

Also, please see my suggestion in the IPFS issue to sign a challenge transaction and validate it backend side to get the IPFS token.

@0xExp-po
Copy link
Contributor Author

0xExp-po commented Dec 6, 2024

I've update the Response data type.

export interface Response<T> {
  data: T | null;
  error: boolean;
  errorCode: number;
  errorMessage: string;
}

We can alert to users with errorMessage.
And according to the errorCode, we can add additional handling action.

Copy link
Owner

@tupui tupui left a comment

Choose a reason for hiding this comment

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

That's great, I will test locally the whole flow again and merge if that works

@@ -47,11 +103,19 @@ async function backend(did) {
"filecoin/offer",
"upload/add",
];
const expiration = Math.floor(Date.now() / 1000) + 10;
const expiration = Math.floor(Date.now() / 1000) + 86400;
Copy link
Owner

Choose a reason for hiding this comment

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

I understand that 10s might cause some issues, but 1 day is not reasonable either. What about 1 min?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, 1min

Copy link
Owner

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Approving now as I tested locally and all works as expected. Great job 👏 Just waiting for you to address either here or in a follow up what we discussed.

Copy link
Owner

Choose a reason for hiding this comment

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

BTW, do we need this file at all now?

@tupui tupui merged commit fb668ef into tupui:main Dec 12, 2024
8 of 12 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.

2 participants