use-ink / contracts-ui

Web application for deploying wasm smart contracts on Substrate chains that include the FRAME contracts pallet
https://contracts-ui.substrate.io/
GNU General Public License v3.0
61 stars 44 forks source link

chore: remove validation on file upload #471

Closed statictype closed 1 year ago

statictype commented 1 year ago

removes the check if a wasm field exists in the abi from the file input validation. this is an old feature before the dry run was implemented and is no longer necessary since the failed dry run will prevent instantiation.

netlify[bot] commented 1 year ago

Deploy Preview for contracts-ui ready!

Name Link
Latest commit 17bebd70448d920cbc631f9edadd4c4ca7ac0c26
Latest deploy log https://app.netlify.com/sites/contracts-ui/deploys/648b3c3f481c90000847a624
Deploy Preview https://deploy-preview-471--contracts-ui.netlify.app/
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 settings.

peetzweg commented 1 year ago

I guess the problem is that it tries to validate the wasm even if isWasmRequired option is set to false. Which throws an error if no wasm is available, I guess? https://github.com/paritytech/contracts-ui/blob/66a276e314ee5c2da85277b7a3eaed7a9667f9aa/src/ui/hooks/useMetadata.ts#L80

This should be fixed if we just move the validation code from outside the if statement into the if statement.

https://github.com/paritytech/contracts-ui/blob/66a276e314ee5c2da85277b7a3eaed7a9667f9aa/src/ui/hooks/useMetadata.ts#L78-L88

cypress[bot] commented 1 year ago

Passing run #123 ↗︎

0 61 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge branch 'master' into ae-remove-wasm
Project: Contracts UI Commit: 17bebd7044
Status: Passed Duration: 02:31 💡
Started: Jun 15, 2023 4:31 PM Ended: Jun 15, 2023 4:34 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

peetzweg commented 1 year ago

Not sure what the cause for this change is? Is there a ticket describing in the issue?

Validation is probably not needed as the instantiation extrinsic dryrun would fail? 🤷

statictype commented 1 year ago

Validation is probably not needed as the instantiation extrinsic dryrun would fail?

exactly

Not sure what the cause for this change is? Is there a ticket describing in the issue?

except the fact it's not needed, removing it allows experimentation with other targets like RISC V