AngelProtocolFinance / angelprotocol-web-app

Repo for Angel Protocol's React web app
https://angelprotocol.io
GNU General Public License v3.0
3 stars 6 forks source link

Investigate the effect of removing `IS_TEST` variable #1830

Closed SovereignAndrey closed 4 months ago

SovereignAndrey commented 1 year ago

CU #: 37q7tff - https://app.clickup.com/t/37q7tff Created on: Dec 6 2022 Original Creator: Nenad Misic

Description

Investigate the effect of removing IS_TEST (and EXPECTED_NETWORK_TYPE ) variable, thus allowing users to connect on testnet networks. We need to see if there is really a need to even limit users from connecting testnet networks.

The variable is defined in env.ts

ap-justin commented 1 year ago

@SovereignAndrey, @misicnenad, @abbasali1994 I've outlined here how we previously handled IS_TEST

Previous setup

wallet's connection drives whether we show testnet or mainnet stuff

Image

disadvantages to this:

  1. there's a need to pass IS_TEST | chainId (redux, would need to resolve this chainId to either testnet or mainnet) as state to redux/query :

example: I connected my wallet to goerli -so the expected network is testnet or 5 to be resolved as testnet . I need to pass this information to redux/query because

Failure to do this would cause unexpected bugs like if marketplace still queries mainnet endowments then donating to those using a wallet connected to goerli would fail and vice-versa.

  1. One way to pass data to queriers (a lot of queriers) is to just include them in their query arguments say.
const endow_details = useEndowDetails({...args, is_test})
const endowments = useAWSEndowQueries({...args, is_test})

Or what was previously done is to just create a redux slice and let an aggregator (which supposedly) should just read state is now also modifying another state. Image

  1. state redux/context is only conveniently accessible to react components. This means that resolution of constants, helpers should be done by their consumers or for case of helpers this data should be passed to them

  2. Not all wallet is reactive: when I change terra wallet to pisco it doesn't emit an event that chain is changed so manual tracking of prev and next chain is required so we only dispatch to chain-slice. Also, if we fail to only dispatch to chain-slice when chain is actually changed - we could go on to render loop

Image

  1. The choice is not explicit. Therefore if user accidentally connects to testnet network but intends to use the app in mainnet we can't tell if something is wrong.

Current setup

really simple, network (is_test) is already known at build time to every part of the app the only drawback is you can't change it once app is built

Image

Simplier modifiable setup

If we still want this simplicity plus the ability to modify whether to use(this has to be explicit: so if user intends to use app for testnet) and then connects to mainnet we could show - "hey we only support goerli for metamask"

and we could even show a banner ---You are currently in test mode --- Image

SovereignAndrey commented 1 year ago

Simplier modifiable setup If we still want this simplicity plus the ability to modify whether to use(this has to be explicit: so if user intends to use app for testnet) and then connects to mainnet we could show - "hey we only support goerli for metamask"

and we could even show a banner ---You are currently in test mode ---

@ap-justin I think this is the way to go. :100: We can re-use the toggle component that we have for profile publication status in the footer in some, unobtrusive manner to allow toggling to testnet for those that want to play around. :+1: