feature: Geo blocking with 451 #382

gbarkhatov · 2024-07-25T19:17:52Z

general health check error: https://i.imgur.com/ozubBzB.png
geo 451 error: https://i.imgur.com/qQOScR4.png

vitsalis

We need to have a separation between the internal representation of the API being available and the fact that we are using the health check endpoint to accomplish that.

src/app/api/apiWrapper.ts

vitsalis · 2024-07-26T08:41:04Z

src/app/api/getHealthCheck.ts

+      (error.response?.status === 451 || error.request.status === 451)
+    ) {
+      return {
+        status: "geoBlocked",


Suggested change

status: "geoBlocked",

status: "geoblocked",

Weird for a status to have camel case

common syntax for such constant usually using upper case letter with _
Is this something we can agree on as a team and adopt for all future changes?
for example, axios use this pattern https://www.npmjs.com/package/axios#error-type

vitsalis · 2024-07-26T08:41:50Z

src/app/components/Connect/ConnectSmall.tsx

@@ -35,6 +40,26 @@ export const ConnectSmall: React.FC<ConnectSmallProps> = ({

  const { coinName, networkName } = getNetworkConfig();

+  const renderHealthCheckTooltip = () => {


This tooltip is not about healthcheck, it's about showing whether someone is geo-blocked or not.

It is health check indeeed, if we get anything besides the normal response we will render an error text OR geo blocking text

vitsalis · 2024-07-26T08:42:52Z

src/app/components/Connect/ConnectSmall.tsx

+        <span
+          className="cursor-pointer text-xs"
+          data-tooltip-id={`tooltip-connect-${healthCheck.status}`}
+          data-tooltip-content={healthCheck.message}


Why are we getting the content for a tooltip directly out of the error returned from an API response? The API response should not specify the content, the API response should specify what went wrong, and based on what went wrong we construct the content in the content component.

vitsalis · 2024-07-26T08:43:49Z

src/app/types/healthCheck.ts

@@ -0,0 +1,4 @@
+export type HealthCheckResult =
+  | { status: "normal"; message: string }


normal, geoblocked and error should be constants.

Otherwise, we could also have an enum, but I understand that's difficult in js.

vitsalis · 2024-07-26T08:45:36Z

src/app/components/Staking/Staking.tsx

+          messages={[
+            "The Bitcoin Staking functionality is not available at the moment.",
+          ]}
+          icon={healthCheckFailed}


This icon has nothing to do with health check. Health check is the endpoint that we use to check whether the API is available. The icon name should be something along the lines of apiNotAvailable

vitsalis · 2024-07-26T08:45:48Z

src/app/components/Staking/Staking.tsx

@@ -83,6 +87,7 @@ export const Staking: React.FC<StakingProps> = ({
  setDelegationsLocalStorage,
  btcWalletBalanceSat,
  availableUTXOs,
+  healthCheck,


Suggested change

healthCheck,

apiAvailable,

@vitsalis you want it only in Staking? Or do you want me to change the healthCheck completely to apiAvailable?

vitsalis

Ideally, I would like an extra layer between healthcheck and the rest of the application, but this might be overengineering, so lgtm.

vitsalis · 2024-07-27T08:41:17Z

src/app/components/Connect/ConnectSmall.tsx

@@ -17,13 +25,15 @@ interface ConnectSmallProps {
  address: string;
  btcWalletBalanceSat?: number;
  onDisconnect: () => void;
+  apiAvailable?: HealthCheckResult;


Why is this ??

It is undefined before we fetch and get into one of the normal - geoblocking - error state

vitsalis · 2024-07-27T08:42:37Z

src/app/components/Connect/ConnectSmall.tsx

@@ -35,6 +45,31 @@ export const ConnectSmall: React.FC<ConnectSmallProps> = ({

  const { coinName, networkName } = getNetworkConfig();

+  const renderApiAvailableTooltip = () => {


Suggested change

const renderApiAvailableTooltip = () => {

const renderApiNotAvailableTooltip = () => {

Let's also have a comment describing what this function's purpose is.

vitsalis · 2024-07-27T08:44:02Z

src/app/components/Connect/ConnectSmall.tsx

+      <button
+        className="btn-primary btn h-[2.5rem] min-h-[2.5rem] rounded-full px-2 text-white md:rounded-lg"
+        onClick={onConnect}
+        disabled={!!address || !isApiNormal}


Let's add a comment denoting that we disable the button if the API is not healthy.

gbarkhatov · 2024-07-27T09:30:12Z

@jrwbabylonchain @jeremy-babylonchain waiting for the 2nd pair of eyes

jrwbabylonlab · 2024-07-29T04:44:15Z

Dumping my thoughts about the PR.
Types:
I generally think it's best to take a hybrid approach on managing the types.
We shall:

Put widely shared types under types
Put domain specific/package specific under the same location as the actual implementation.

For the health check case, I feel this is quite confusing using the healthcheck result type outside of the api client. why would react component need to have knowledge of "backend healthcheck"?

The more i think about it, the more i think it's due to the fact that our current getHealthCheck is actually mixing up client layer and service layer. Let's breakdown in details what those two layers do:

Client layer: No business logic, it just setup axios and http client configurations (connection pool, timeout, analytics etc) to making the call. The healthcheck types only lives here.
Service layer: All our business logic and custom types sit here. This layer will call the healtcheck client method, then turn it into GeoBlock related terms, objects and types.

To summarise, I think we shall take this healthcheck as a good opportunity to standandarise the layers and avoid involving API specific types across the repo. i.e this shall not appear https://github.com/babylonchain/simple-staking/pull/382/files#diff-8be97df2c2968a8d06ade0ed70c78ddde9ff115505d6335ecff9119d47b47dbfR28 it shall be replaced with more domain specific things such as IsGeoBlocked etc.

Dependency

We are injecting the apiAvailable in many different places. What we actually need is a boolean value to indicate whether it is geoBlocked. Why not consider putting it high put in the react chain (useContext) so that we have less props to manage? This will make code cleaner and easier to write tests.

gbarkhatov · 2024-07-29T16:20:29Z

@jrwbabylonlab

I will check whether or not I will apply it as a more domain specific naming and placement, separation of concerns. But the same approach with "good opportunity to start making better stuff" might be applied to new additions to 700 lines of code Staking.tsx
apiAvailable is used in 2 places - Header and Staking. I would not put any state to global (whether it's context or redux or whatever) unless I have to. Looking at UI side, I believe at some point of time we will have only one place to connect the wallet (not we have UI connect duplication), so as later on when we refactor and improve I think we might cut the amount of context providers. I'll move this PR to labs

initial

2e2a866

gbarkhatov requested review from vitsalis, jrwbabylonlab and jeremy-babylonlabs July 25, 2024 19:17

gbarkhatov mentioned this pull request Jul 25, 2024

feature: Geo blocking #378

Closed

vitsalis requested changes Jul 26, 2024

View reviewed changes

apiAvailable, no use of wrapper, enums, messages, statuses

9226b80

gbarkhatov requested a review from vitsalis July 26, 2024 09:20

vitsalis approved these changes Jul 27, 2024

View reviewed changes

gbarkhatov added 2 commits July 27, 2024 11:28

renderApiNotAvailableTooltip + comment

0d314c4

disabled button comment

f8b288f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Geo blocking with 451 #382

feature: Geo blocking with 451 #382

gbarkhatov commented Jul 25, 2024

vitsalis left a comment

vitsalis Jul 26, 2024

gbarkhatov Jul 26, 2024

jrwbabylonlab Jul 29, 2024

vitsalis Jul 26, 2024

gbarkhatov Jul 26, 2024

gbarkhatov Jul 26, 2024

vitsalis Jul 26, 2024

gbarkhatov Jul 26, 2024

gbarkhatov Jul 26, 2024

vitsalis Jul 26, 2024

gbarkhatov Jul 26, 2024

vitsalis Jul 26, 2024

gbarkhatov Jul 26, 2024

vitsalis Jul 26, 2024

gbarkhatov Jul 26, 2024

gbarkhatov Jul 26, 2024

vitsalis left a comment

vitsalis Jul 27, 2024

gbarkhatov Jul 27, 2024

vitsalis Jul 27, 2024

vitsalis Jul 27, 2024

gbarkhatov Jul 27, 2024

vitsalis Jul 27, 2024

gbarkhatov Jul 27, 2024

gbarkhatov commented Jul 27, 2024

jrwbabylonlab commented Jul 29, 2024 •

edited

Loading

gbarkhatov commented Jul 29, 2024

		@@ -35,6 +40,26 @@ export const ConnectSmall: React.FC<ConnectSmallProps> = ({

		const { coinName, networkName } = getNetworkConfig();

		const renderHealthCheckTooltip = () => {

		@@ -0,0 +1,4 @@
		export type HealthCheckResult =
		\| { status: "normal"; message: string }

		@@ -35,6 +45,31 @@ export const ConnectSmall: React.FC<ConnectSmallProps> = ({

		const { coinName, networkName } = getNetworkConfig();

		const renderApiAvailableTooltip = () => {

	const renderApiAvailableTooltip = () => {
	const renderApiNotAvailableTooltip = () => {

feature: Geo blocking with 451 #382

Are you sure you want to change the base?

feature: Geo blocking with 451 #382

Conversation

gbarkhatov commented Jul 25, 2024

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbarkhatov commented Jul 27, 2024

jrwbabylonlab commented Jul 29, 2024 • edited Loading

gbarkhatov commented Jul 29, 2024

jrwbabylonlab commented Jul 29, 2024 •

edited

Loading