Validate the calculated Bitcoin deposit address and track it in Sentry #257
Conversation
- create a directory for hooks - remove SDK parent directory for sentry
SDKs don't automatically capture messages, but we can capture them manually.
VITE_SENTRY_SUPPORT was a boolean but returned a string when parsed. We have to make sure to convert it to the desired type when using it in our code.
| @@ -1,7 +1,6 @@ | |||
| /// <reference types="vite/client" /> | |||
|
|
|||
| interface ImportMetaEnv { | |||
| readonly VITE_SENTRY_SUPPORT: boolean | |||
There was a problem hiding this comment.
Why did we remove it? Looks like we use this env variable in useSentry hook.
There was a problem hiding this comment.
That's right. However, take a look here. Even though we set the type VITE_SENTRY_SUPPORT here, it will return a string. I treated it as a temporary solution for now. We need to check/improve working with env vars when these are of boolean type. I will create a separate task for this.
As shown above, VITE_SOME_KEY is a number but returns a string when parsed. The same would also happen for boolean env variables. Make sure to convert to the desired type when using it in your code.
kkosiorowska
force-pushed
the
validate-deposit-address
branch
from
26de39d to
c6bea23
Compare
kkosiorowska
force-pushed
the
validate-deposit-address
branch
from
c6bea23 to
c8478b8
Compare
dapp/src/sentry/index.ts
Outdated
There was a problem hiding this comment.
Maybe it's worth considering on creating one common catalog for Sentry and acre-react if we don't want to keep Sentry longer in the SDK directory., eg. src/vendor/.
CC: @r-czajkowski
There was a problem hiding this comment.
Previously, the sentry directory was nested in the SDK directory. I took it a level up simply because it seems to me that the structure will be more clear. But I'm open to all suggestions
There was a problem hiding this comment.
Sentry should not be a part of the acre-react. The dapp should care about the sentry or other integrations.
There was a problem hiding this comment.
@r-czajkowski I didn't say that it should be part of acre-react, I just thought about putting it in one folder:
src:
-vendor:
-- acre-react
-- sentry
|
|
||
| const response = await axios.get<{ address: string }>( | ||
| url, | ||
| { timeout: 10000 }, // 10s |
There was a problem hiding this comment.
| { timeout: 10000 }, // 10s | |
| { timeout: ONE_SEC_IN_MILLISECONDS * 10 }, |
| response: unknown | ||
| }> { | ||
| const endpoint = | ||
| "https://us-central1-keep-prd-210b.cloudfunctions.net/verify-deposit-address" |
There was a problem hiding this comment.
let's move it out of this function
| depositAddress: string, | ||
| network: BitcoinNetwork, | ||
| ): Promise<{ | ||
| status: "valid" | "invalid" | "error" |
There was a problem hiding this comment.
hmm We don't really use this type anywhere else. Therefore, I wonder if it makes sense. 🤔 This status is strictly related only to verifyDepositAddress.
| depositor.identifierHex | ||
| }/${blindingFactor.toString()}/${refundPublicKeyHash.toString()}/${refundLocktime.toString()}` | ||
|
|
||
| const url = extraData ? `${baseUrl}/${extraData.toString()}` : baseUrl |
There was a problem hiding this comment.
Let's create a helper function to generate this url and baseUrl which will take the extraData parameter.
Closes #238
This PR validates the calculated Bitcoin deposit address and tracks this action in Sentry. It is based on the solution implemented in this PR.
The verification outcome is attached to the telemetry data as two additional fields:
verificationStatuswhich can have three different valuesvalid: verification service responded with the same address as the generated oneinvalid: verification service responded with a different addresserror: request or verification service errorverificationResponsewhich holds the complete request outcome