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

Refactor CreateProposalTemplateForm into generic ProposalBuilder #1524

Merged
merged 20 commits into from
Apr 14, 2024

Conversation

mudrila
Copy link
Contributor

@mudrila mudrila commented Apr 8, 2024

This PR refactors CreateProposalTemplateForm into "general use" ProposalBuilder component and uses that component on CreateProposalTemplatePage and CreateProposalPage. This PR will accumulate other PRs related to this chain of improvements/refactoring.

…more generic to be compatible with creating regular proposal
@mudrila mudrila added enhancement New feature or request maintenance Keep the lights on labels Apr 8, 2024
@mudrila mudrila self-assigned this Apr 8, 2024
Copy link

netlify bot commented Apr 8, 2024

Deploy Preview for fractal-dev ready!

Name Link
🔨 Latest commit b9514d9
🔍 Latest deploy log https://app.netlify.com/sites/fractal-dev/deploys/661be82fe9eddf00080af3d2
😎 Deploy Preview https://deploy-preview-1524.app.dev.fractalframework.xyz
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@DarksightKellar DarksightKellar left a comment

Choose a reason for hiding this comment

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

LGTM.

#1525 should be merged first

Da-Colon
Da-Colon previously approved these changes Apr 9, 2024
Copy link
Contributor

@Da-Colon Da-Colon left a comment

Choose a reason for hiding this comment

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

Awesome. Easy to follow. Just a few code moves, name and file changes. LGTM

mudrila added 3 commits April 9, 2024 17:40
…that-just-sends-assets

Allow creating proposal with no function name / signature specified
…ilder-for-create-proposal-page

Use ProposalBuilder on Create Proposal page
@Da-Colon Da-Colon dismissed their stale review April 9, 2024 16:56

Going to dismiss for the moment, to do a full live testing review, now that the branches are merged in

@tomstuart123
Copy link

Hi @mudrila

Did a few tests this morning (results below). Note - I'd like to get @Da-Colon / @adamgall in the UI creating proposals in the UI too to i) try some more complex Txs ii) try some from a multisig DAO.

Azorious DAO Proposal:

Proposal #1 - Tried to approve token for disperse
Screenshot 2024-04-10 at 11 43 12
I got this error when I clicked create proposal UNLESS I had an ETH value specified
Screenshot 2024-04-10 at 11 43 55
When I specified 0 ETH, the proposal was created and could be voted on and executed.

QUESTION - Based on the above flow, shouldn't ETH value have an '*' next to it in the UI so the user knows it is mandatory
Screenshot 2024-04-10 at 12 03 42

Proposal #2 - Tried to create proposal to disperse token

So in summary everything seemed to work as intended but I think we should make it clearer that the ETH Value is now mandatory and do some more tests with multisig daos and sending ethvalue

Copy link
Contributor

@Da-Colon Da-Colon left a comment

Choose a reason for hiding this comment

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

I noticed the helper text for eth value is 1.2 is this still correct? or is this a suppose to be in wei?

Copy link
Contributor

@Da-Colon Da-Colon left a comment

Choose a reason for hiding this comment

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

Awesome. Tested and works well, sides from the known ETH Value issues already discussed.

I tested using the WETH contract. with through all three steps seperately (deposit, approve, withdraw), then tested the multitransaction with two deposits.

@tomstuart123
Copy link

I noticed the helper text for eth value is 1.2 is this still correct? or is this a suppose to be in wei?

good point, can we confirm?

Also @mudrila did you say at standup that my asterix point above is solved by the 'hot fix' PR?

Hotfix for missing bigNumberValue on ethValue while creating proposal from template
@mudrila
Copy link
Contributor Author

mudrila commented Apr 11, 2024

I noticed the helper text for eth value is 1.2 is this still correct? or is this a suppose to be in wei?

Yup, that's correct - we're exponentiating this later on

@mudrila mudrila requested a review from tomstuart123 April 11, 2024 13:45
@tomstuart123
Copy link

Hi @mudrila

Did a few tests this morning (results below). Note - I'd like to get @Da-Colon / @adamgall in the UI creating proposals in the UI too to i) try some more complex Txs ii) try some from a multisig DAO.

Azorious DAO Proposal:

Proposal #1 - Tried to approve token for disperse Screenshot 2024-04-10 at 11 43 12 I got this error when I clicked create proposal UNLESS I had an ETH value specified Screenshot 2024-04-10 at 11 43 55 When I specified 0 ETH, the proposal was created and could be voted on and executed.

QUESTION - Based on the above flow, shouldn't ETH value have an '*' next to it in the UI so the user knows it is mandatory Screenshot 2024-04-10 at 12 03 42

Proposal #2 - Tried to create proposal to disperse token

So in summary everything seemed to work as intended but I think we should make it clearer that the ETH Value is now mandatory and do some more tests with multisig daos and sending ethvalue

@mudrila - I tried to redo the same two proposals above.

The first went without any issue

The second wouldn't execute and I got proposal failed with the below error message.
Screenshot 2024-04-12 at 14 53 10

Any ideas?

@tomstuart123
Copy link

@mudrila apologies, the bug above was me not wrapping the value / address with []... AGAIN.

My bad, the proposal got created. I just want to do a few more multisig create proposals to match these azorious ones. I'll approve my end of day

@adamgall
Copy link
Member

@mudrila can you please handle these conflicts? The "bigint" PR was merged, and you have quite a few changes to various BigNumber properties in this PR.

@adamgall
Copy link
Member

Oops, forgot to leave a comment on the review I just left. Just one small change with the change in i18n.

There's a lot happening in this PR, but the changes seem relatively straightforward, so I have no major comments or suggestions. Nice job!

Copy link
Member

@adamgall adamgall left a comment

Choose a reason for hiding this comment

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

Actually, just going to leave an "Approve" on here. Would like that English string to get cleaned up though.

@mudrila mudrila merged commit 12b9753 into develop Apr 14, 2024
7 checks passed
@mudrila mudrila deleted the feature/#1363-generic-templates-builder branch April 14, 2024 14:33
@tomstuart123
Copy link

@mudrila apologies, the bug above was me not wrapping the value / address with []... AGAIN.

My bad, the proposal got created. I just want to do a few more multisig create proposals to match these azorious ones. I'll approve my end of day

I @mudrila I got the multisig testing done this half,. To add to the above it worked:

  1. across three different contracts. 2) with and without eth values

will approve this now

@tomstuart123
Copy link

hahah just realised it was merged yesterday. Nice :)

@adamgall adamgall mentioned this pull request Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request maintenance Keep the lights on
Projects
None yet
5 participants