Bug: 422 status code when proposing transaction from an app #2706
Conversation
* Handle chunk loading error + add Suspense types * Remove hook/expiry functions from error boundary * Add account to CLA * Fix ESLint * Fix React import * Extract handleChunkError + test it * Fix tests * Return handleChunkError boolean + alter no. format * Move const to beginning of fn + don't export key * Update test to check for boolean return * Adjust sessionStorage key const
…ert-int-to-string
|
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
| @@ -98,7 +98,8 @@ export const ReviewConfirm = ({ | |||
| [txs, isMultiSend], | |||
| ) | |||
| const txValue: string | undefined = useMemo( | |||
| () => (isMultiSend ? '0' : txs[0]?.value && parseTxValue(txs[0]?.value)), | |||
| // Value is converted to string because numbers were allowed in the safe-apps-sdk v1, so 0 and anything would evaluate as false | |||
| () => (isMultiSend ? '0' : txs[0]?.value.toString() && parseTxValue(txs[0]?.value)), | |||
There was a problem hiding this comment.
What happens if value is undefined?
There was a problem hiding this comment.
If you're expecting undefined, I would suggest optionally chain everything: txs[0]?.value?.toString().
There was a problem hiding this comment.
https://github.com/gnosis/safe-apps-sdk/blob/4ce9a16b2d8f4133f6a7e7fbc25f422210cba786/src/types.ts#L35 the type signature doesn't support undefined, actually, it doesn't even support numbers 😅
Currently, it'd error. I think app developers should test their apps, if we'll account for each possible type things will get crazy.
There was a problem hiding this comment.
@mikheevm but how it is currently this will crash the whole web app, right?
There was a problem hiding this comment.
I mean I am fine that we error, but then we should properly check what is send to the interface. I would also expect to see tests for something like this.
There was a problem hiding this comment.
if we'll account for each possible type things will get crazy
I tend to agree but we don't have to check for wrong types, we need to check for the expected types.
In this case if (typeof value !== 'string') { throw new CodedException('Wrong type used for value') }.
Maybe we could include some runtime type-checker for the apps API, similar to PropTypes in React? And do it in a declarative fashion?
There was a problem hiding this comment.
we have a validation function that checks exactly that: https://github.com/gnosis/safe-react/blob/dev/src/routes/safe/components/Apps/components/ConfirmTxModal/index.tsx#L26
There was a problem hiding this comment.
Found this: https://github.com/codemix/babel-plugin-typecheck
And this: https://github.com/fabiandev/ts-runtime
There was a problem hiding this comment.
Then undefined should never get that far 👍
|
So I was able to create a "Redeem" tx in Aave v1. Didn't execute it since is really expensive to do so, but given that in the original issue the error happens by just trying to create the tx it seems that what I did is enough. Safe: https://pr2706--safereact.review.gnosisdev.com/mainnet/app/#/safes/0x8675B754342754A30A2AeF474D114d8460bca19b/transactions
|
|
@francovenica awesome, this should be enough :) |
|
How do we proceed with the release? @katspaugh |
|
Let's merge this and make a release as usual, @mikheevm. |
What it solves
Resolves #2705
How this PR fixes it
In safe-apps-sdk v0.x.x it was allowed to use an integer for specifying transaction value. This is not the case anymore.
Some apps are still using an old sdk version and therefore this line of code would evaluate to 0:
https://github.com/gnosis/safe-react/blob/93ce1803605198af282a18d8fb2c3b7227dbe60f/src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx#L101
The backend expects a string, so the transaction will be rejected by the backend
How to test it
Try using aave v1 app