Variable eth value in proposal template #1510

mudrila · 2024-04-01T17:11:30Z

Logic of these changes is following:
We're displaying ETH value input in ProposalTemplateModal if there's no value at all, but also if "Show All" switcher turned on. This logic might be debatable as we're disabling parameters inputs if value is set from the template.

For testing

Target the WETH9 contract on Sepolia
0xfff9976782d46cc05630d1f6ebab18b2324d6b14

…o "uncontrolled to controlled switching"

…d, not zero

…nput

netlify · 2024-04-01T17:11:47Z

✅ Deploy Preview for fractal-dev ready!

Name	Link
🔨 Latest commit	`95e8924`
🔍 Latest deploy log	https://app.netlify.com/sites/fractal-dev/deploys/660b11329b77c90008663565
😎 Deploy Preview	https://deploy-preview-1510.app.dev.fractalframework.xyz
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

adamgall

Was able to create the templates with a "variable ETH value", but having trouble actually using them. Would have been hard to explain, so I created two videos showing two different bugs:

https://youtu.be/6JZq5-tXuZo

https://youtu.be/eAdGNxBGyZQ

Da-Colon

I was able to create the template and when I went to use it got a estimate gas error. Is this because there is a ethValue added or should this pass even if I add ETH Value

(added 0.0001)

Da-Colon · 2024-04-01T20:23:29Z

src/components/ui/forms/BigNumberInput.tsx

I'm very nervous about changes to this file. This file is used pretty heavy in all the other forms in the app. Why are these changes needed? I ask this because this Component is really sensitive to changes as it will affect every other place it is used.

That being said I have done some live testing 'creating a DAO' and don't see any problems with the BigNumber Inputs.

adamgall

Comments

The Good

Am able to submit a variable amount of ETH along with a transaction created via Proposal Template

Happy path seemed to work well! I created a proposal template that called the deposit function on the WETH9 contract. I was able to use this template and manually input an amount of ETH (0.0001), which was then successfully wrapped into WETH.

The Not So Good

"Submit and create proposal" button

"Submit and Create Proposal" button IMO should only be clickable if all inputs are populated. In the screenshot below, clicking the button results in an toast error.

What's the effort involved to do this?

Proposal (from template) that simply "sends ETH" fails

Creating a proposal template with no function specified on the target contract works, but trying to create a proposal with that template fails (very likely because there is no function name specified in the template JSON).

See these two templates. Both target the WETH9 contract. The first one wraps ETH by calling the deposit function, the intention of the second one is to wrap ETH by sending ETH directly to the contract. I was able to create both of these through Fractal's UI.

[
  {
    "title": "WETH9 Deposit thru function",
    "description": "Deposit ETH into the WETH9 contract through calling the `deposit` function.",
    "transactions": [
      {
        "targetAddress": "0xfff9976782d46cc05630d1f6ebab18b2324d6b14",
        "ethValue": {
          "value": ""
        },
        "functionName": "deposit",
        "parameters": []
      }
    ]
  },
  {
    "title": "Wrap ETH to WETH w/o `deposit` function",
    "description": "Just send ETH directly to the contract and trigger the fallback function.",
    "transactions": [
      {
        "targetAddress": "0xfff9976782d46cc05630d1f6ebab18b2324d6b14",
        "ethValue": {
          "value": ""
        },
        "functionName": "",
        "parameters": []
      }
    ]
  }
]

Here is the error in Chrome's console when clicking "Submit and create proposal" while trying to use the second template:

console.js:213 Error: invalid identifier "" (argument="value", value="", code=INVALID_ARGUMENT, version=abi/5.7.0)
    at _Logger.makeError (chunk-ANHLTHON.js?v=2b819a64:207:23)
    at _Logger.throwError (chunk-ANHLTHON.js?v=2b819a64:216:20)
    at _Logger.throwArgumentError (chunk-ANHLTHON.js?v=2b819a64:219:21)
    at verifyIdentifier (chunk-ANHLTHON.js?v=2b819a64:8003:14)
    at _FunctionFragment.fromObject (chunk-ANHLTHON.js?v=2b819a64:8431:17)
    at _FunctionFragment.fromString (chunk-ANHLTHON.js?v=2b819a64:8467:34)
    at _Fragment.fromString (chunk-ANHLTHON.js?v=2b819a64:8214:35)
    at _Fragment.from (chunk-ANHLTHON.js?v=2b819a64:8184:28)
    at chunk-ANHLTHON.js?v=2b819a64:8690:27
    at Array.map (<anonymous>) Array(0)

For reference, here are the relevant details from the DAO being used to test this:

Network: Sepolia
Address: 0x0FAB989675bcbE3adFBAE4eC8E54061CcCE85020
IPFS Hash for Proposal Templates: https://cloudflare-ipfs.com/ipfs/QmdcwXNLeGo3ibqzYHc2oNHRkLVLcyfhVY7tN9ASgs5enu (see the last two)

@mudrila @Da-Colon @tomstuart123 do you think either of these two issues I'm describing fall under the umbrella of this PR, or should we simply open new issues for them and tackle them later?

adamgall · 2024-04-03T13:23:32Z

I was able to create the template and when I went to use it got a estimate gas error. Is this because there is a ethValue added or should this pass even if I add ETH Value

(added 0.0001)

@Da-Colon Can you please give steps to reproduce this?

Da-Colon · 2024-04-03T14:35:14Z

@mudrila @Da-Colon @tomstuart123 do you think either of these two issues I'm describing fall under the umbrella of this PR, or should we simply open new issues for them and tackle them later?

This feels like separate issues to the goal of this PR.

Da-Colon · 2024-04-03T15:04:53Z

I was able to create the template and when I went to use it got a estimate gas error. Is this because there is a ethValue added or should this pass even if I add ETH Value
(added 0.0001)

You answered my question yesterday that I can't do that. where you told us that testing with the WETH contract was best for this

adamgall · 2024-04-03T15:08:39Z

I was able to create the template and when I went to use it got a estimate gas error. Is this because there is a ethValue added or should this pass even if I add ETH Value

(added 0.0001)

@Da-Colon Can you please give steps to reproduce this?

You answered my question yesterday that I can't do that. where you told us that testing with the WETH contract was best for this

@Da-Colon are you saying that you can't recreate the original issue you called out? Or are you saying you can recreate the issue, but we said it's known and expected? Sorry, I'm confused what this reply means.

Da-Colon · 2024-04-03T15:10:15Z

@Da-Colon are you saying that you can't recreate the original issue you called out? Or are you saying you can recreate the issue, but we said it's known and expected? Sorry, I'm confused what this reply means.

Saying The answer to my question was that I was incorrectly testing and that was going to fail because the function I'm calling isn't payable.

adamgall · 2024-04-03T15:11:42Z

@Da-Colon are you saying that you can't recreate the original issue you called out? Or are you saying you can recreate the issue, but we said it's known and expected? Sorry, I'm confused what this reply means.

Saying The answer to my question was that I was incorrectly testing and that was going to fail because the function I'm calling isn't payable.

Ohhhh that's right. Thank you

adamgall · 2024-04-03T15:12:25Z

@mudrila @Da-Colon @tomstuart123 do you think either of these two issues I'm describing fall under the umbrella of this PR, or should we simply open new issues for them and tackle them later?

This feels like separate issues to the goal of this PR.

I tend to agree...

Will approve this PR and open up issues for the other things I'm seeing.

tomstuart123 · 2024-04-03T18:33:19Z

duplicating this from here - https://decentdao.slack.com/archives/C06QZTUTKRC/p1712169164168019

hey @trainface / @Kirill - I've struggled to find time to test PR 1510 today and want to spend any loose time tomorrow AM drafting the rest of the comms to Shutter out so we can push to Prod asap.

I'm not sure I should block this 1510PR though as - tbh @trainface - I would only be testing the same case as you talked me through with WETH9... and it looks like via the comments you've already proven this.

I agree, though, we should get the templates 'lack of function' fixed as a part of @Kirill’s next effort. In this case though, recommend @trainface and @Epicbadtiming act as reviewers.

Have duplicated this comment in github and can you confirm this is fine?

adamgall and others added 8 commits March 21, 2024 10:48

Make inputValue start as an empty string, not undefined, so there's n…

76b02d1

…o "uncontrolled to controlled switching"

If user deletes all values from input, set bigNumberValue to undefine…

d099424

…d, not zero

Set default ETH value as "empty" for new proposal templates

566060a

Add language to helper text telling user to leave empty for a blank i…

dc97a2b

…nput

Merge branch 'develop' into enhancement/#1207-variable-eth-value

bd89d66

Merge branch 'develop' into enhancement/#1207-variable-eth-value

a678564

Add input to ProposalTemplateModal to enter ETH value

b0b5d3c

Prettier

63b71c3

mudrila added the enhancement New feature or request label Apr 1, 2024

mudrila requested review from adamgall, tomstuart123 and Da-Colon April 1, 2024 17:11

mudrila self-assigned this Apr 1, 2024

adamgall requested changes Apr 1, 2024

View reviewed changes

Do not trim extra decimal zeros

95e8924

mudrila requested a review from adamgall April 1, 2024 19:56

Da-Colon reviewed Apr 1, 2024

View reviewed changes

adamgall reviewed Apr 3, 2024

View reviewed changes

adamgall approved these changes Apr 3, 2024

View reviewed changes

This was referenced Apr 3, 2024

"Submit and create proposal" button #1512

Closed

Proposal (from template) that simply "sends ETH" fails #1513

Closed

Da-Colon approved these changes Apr 3, 2024

View reviewed changes

mudrila merged commit 99cf8f3 into develop Apr 4, 2024
7 checks passed

mudrila deleted the enhancement/#1207-variable-eth-value branch April 4, 2024 16:17

adamgall mentioned this pull request Apr 5, 2024

Release/2024-04-08 Part 1: The Merge into prod #1521

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Variable eth value in proposal template #1510

Variable eth value in proposal template #1510

mudrila commented Apr 1, 2024 •

edited by adamgall

Loading

netlify bot commented Apr 1, 2024 •

edited

Loading

adamgall left a comment

Da-Colon left a comment

Da-Colon Apr 1, 2024

adamgall left a comment •

edited

Loading

adamgall commented Apr 3, 2024

Da-Colon commented Apr 3, 2024 •

edited

Loading

Da-Colon commented Apr 3, 2024 •

edited

Loading

adamgall commented Apr 3, 2024 •

edited

Loading

Da-Colon commented Apr 3, 2024 •

edited

Loading

adamgall commented Apr 3, 2024

adamgall commented Apr 3, 2024

tomstuart123 commented Apr 3, 2024

Variable eth value in proposal template #1510

Variable eth value in proposal template #1510

Conversation

mudrila commented Apr 1, 2024 • edited by adamgall Loading

For testing

netlify bot commented Apr 1, 2024 • edited Loading

✅ Deploy Preview for fractal-dev ready!

adamgall left a comment

Choose a reason for hiding this comment

Da-Colon left a comment

Choose a reason for hiding this comment

Da-Colon Apr 1, 2024

Choose a reason for hiding this comment

adamgall left a comment • edited Loading

Choose a reason for hiding this comment

The Good

Am able to submit a variable amount of ETH along with a transaction created via Proposal Template

The Not So Good

"Submit and create proposal" button

Proposal (from template) that simply "sends ETH" fails

adamgall commented Apr 3, 2024

Da-Colon commented Apr 3, 2024 • edited Loading

Da-Colon commented Apr 3, 2024 • edited Loading

adamgall commented Apr 3, 2024 • edited Loading

Da-Colon commented Apr 3, 2024 • edited Loading

adamgall commented Apr 3, 2024

adamgall commented Apr 3, 2024

tomstuart123 commented Apr 3, 2024

mudrila commented Apr 1, 2024 •

edited by adamgall

Loading

netlify bot commented Apr 1, 2024 •

edited

Loading

adamgall left a comment •

edited

Loading

Da-Colon commented Apr 3, 2024 •

edited

Loading

Da-Colon commented Apr 3, 2024 •

edited

Loading

adamgall commented Apr 3, 2024 •

edited

Loading

Da-Colon commented Apr 3, 2024 •

edited

Loading