Remove coin machine & whitelist extensions #3663

danbr · 2022-07-21T12:54:17Z

Description

This PR removes coin machine & whitelist extensions from the Dapp (combining #3531 & #3532).

Extensions authorised to be used within the Dapp are defined here:
https://github.com/JoinColony/colonyDapp/blob/refactor/3532-remove-whitelist-coinmachine/src/modules/dashboard/components/Extensions/index.ts#L7

export const dappExtensions = [
  Extension.OneTxPayment,
  Extension.VotingReputation,
];

I have removed the following for both coin machine & whitelist extensions :

There are many changes in this PR, many areas were touched. I have tested many paths through the app without discovering any issues or anomalies.

Resolves #3531 & #3532

ArmandoGraterol

Nice work! There are still some places that need to be updated:

In the actionTypes file there are some WHITELIST_UPDATE types that correspond to the whitelist extension.
The WhitelistAddresses and WhitelistMembersListExtraContent components were only used by the whitelist extension.
src/types/extensions.ts only contains a type that was used by the coin machine, so that file can be deleted.
These dependencies can now be uninstalled: @synaps-io/verify.js, react-confetti, react-textfit, and react-twitter-widgets

danbr · 2022-07-25T06:27:26Z

Nice work! There are still some places that need to be updated:

In the actionTypes file there are some WHITELIST_UPDATE types that correspond to the whitelist extension.

The WhitelistAddresses and WhitelistMembersListExtraContent components were only used by the whitelist extension.

src/types/extensions.ts only contains a type that was used by the coin machine, so that file can be deleted.

These dependencies can now be uninstalled: @synaps-io/verify.js, react-confetti, react-textfit, and react-twitter-widgets

Thanks for your comments, they're very helpful. In fact i was planning on adding a comment on WHITELIST_UPDATE in the PR. I wasnt sure if it was being used..... Originally i removed it, and it caused a silly amount of errors.
I'll implement your suggestions.

willm30

When hitting /extensions/CoinMachine and /extensions/Whitelist in the URL, the app crashes with the following errors instead of redirecting:

Coin Machine

Whitelist

ArmandoGraterol

Looking great! A lot of deleted code, kind of satisfying :)

ArmandoGraterol · 2022-07-25T17:31:36Z

When hitting /extensions/CoinMachine and /extensions/Whitelist in the URL, the app crashes with the following errors instead of redirecting:

Coin Machine

Whitelist

@willm30 That's what happens if you hit that URL with any extension that doesn't exist. It's not okay that it's happening but it's not something introduced by this PR.

It's something that could be dealt with in this PR if Dan wants to, but it can perfectly be its own Maintenance issue.

willm30 · 2022-07-25T18:38:19Z

It's something that could be dealt with in this PR if Dan wants to, but it can perfectly be its own Maintenance issue.

Got it. Made the issue for it here: #3678

chinins

Very comprehensive work - impressive! You probably needed a lot of patience 🙈

I left a couple of comments of small things that can be removed, it's very hard to catch them. I only know because I implemented some of it.

chinins · 2022-07-25T19:42:32Z

src/data/staticData/extensionData.tsx

-import { DEFAULT_NETWORK_INFO } from '~constants';
-import { Address, WhitelistPolicy } from '~types/index';
-import { CM_BLOG_POST, CM_GOOGLE_SHEET, CM_DESCRIPTION } from '~externalUrls';
+import { Address } from '~types/index';


I can't comment directly on the line because it was not changed, so will comment for the file. You can remove ColonyPolicySelector in ExtensionParamType - it's for whitelist as far as I remember and we are not using it anywhere

you can remove orderNumber and options from ExtensionInitParams, it was only for coin machine and whitelist respectively.

I think you can also remove disabled from ExtensionInitParams & from params.map in ExtensionSetup. I think it was only used for whitelist policy selector (when whitelist agreement was not an option). But please test

I think tokenLabel is not needed in ExtensionInitParams either.

I think that you can also remove fieldName from ExtensionInitParams & from ExtensionSetup

And I think you can remove ExtensionParamType.TokenSelector from both files

From ExtensionData I think you can remove:

address

header

descriptionExtended (from ExtensionSetup too)

descriptionLinks (from ExtensionSetup too)

tokenContractAddress (from ExtensionSetup too)

info

extraInitParams (from ExtensionSetup too)

enabledExtensionBody

willm30 · 2022-07-26T20:16:01Z

@danbr As I was working through this issue I noticed references to whitelist in a few places, e.g. the processedColony resolver. I have touched as little as possible in that PR so as to maintain separation of work but did need to tweak a few things so I've used this branch as the base. In the getProcessedColony function for example, I left whitelistActivated in so am bringing to your attention.

I also notice a few search results in VS code when I do a text search for whitelist or WhitelistDialog. You might be aware of these already but also flagging just in case (the 'ManageWhitelistDialog' being the most obvious one).

danbr · 2022-07-27T06:30:39Z

@danbr As I was working through this issue I noticed references to whitelist in a few places, e.g. the processedColony resolver. I have touched as little as possible in that PR so as to maintain separation of work but did need to tweak a few things so I've used this branch as the base. In the getProcessedColony function for example, I left whitelistActivated in so am bringing to your attention.

I also notice a few search results in VS code when I do a text search for whitelist or WhitelistDialog. You might be aware of these already but also flagging just in case (the 'ManageWhitelistDialog' being the most obvious one).

@willm30 Thank you for your notification. There will still be code using whitelist terms but its not associated with the extension .
Previously there was both a whitelist extension and a 'verified recipient' feature that provides an address book and implementation of a whitelist (outside of the extension). Only the whitelist extension was removed.

rdig

Very nice! This obviously took a lot of work! 💯

I've left some notes on what can be improved and stuff that was missed and needs to be removed.

Since I can't comment on the file itself, I'll leave this note here:

The authenticateKYC method from src/api/auth.ts needs to be also removed since it's not being used anymore

Either way I'm pre-approving this!

rdig · 2022-07-27T22:34:45Z

src/modules/dashboard/components/Extensions/index.ts

+export const dappExtensions = [
+  Extension.OneTxPayment,
+  Extension.VotingReputation,
+];


So I think we can simplify this even further.

Remove this

Remove Unknown from the extensions map inside data/staticData/extensionData.tsx (it was set up as an fallback, but we never ended up using it)

Refactor the extensions map to use Extension.<ExtensionName> enums as keys (instead of them being manually typed)

Use that array as the source for what extensions to surface in the dapp

It actually makes sense since if that mapping doesn't have an entry for the extension, it won't have any data to display when trying to render the extension's card on the extension's page.

So it's only fitting that we use this object to also control the "supported" extensions inside the dapp

(maybe leave a comment about this, that we're using this object to control the extensions display)

rdig · 2022-07-27T22:46:54Z

src/modules/core/components/CSVUploader/CSVUploaderItem.tsx


 const MSG = defineMessages({
  removeCSVText: {
-    id: 'dashboard.Whitelist.CSVUploader.CSVUploaderItem.removeCSVText',
+    id: 'core.CSVUploader.CSVUploaderItem.removeCSVText',


Good attention to details :)

rdig · 2022-07-27T22:49:39Z

src/modules/dashboard/components/Extensions/utils.ts

-  actionType: 'SUBMIT' | 'ERROR' | 'SUCCESS',
-  extensionId: string,
-) => {
+export const getButtonAction = (actionType: 'SUBMIT' | 'ERROR' | 'SUCCESS') => {


Again, nice catch. No reason in keeping it around

rdig · 2022-07-27T22:50:15Z

src/modules/dashboard/components/Extensions/Extensions.tsx

+  // @NOTE get list of installed extensions that are only allowed to be
+  // used within the dapp


See my note about using the object from static data for this

rdig · 2022-07-27T22:51:43Z

src/modules/dashboard/components/Extensions/Extensions.tsx

+                  // be careful to use matching data with both
+                  // installedExtensionsData & dappInstalledExtensions
+                  installedExtension={getDappInstalledExtension(idx)}


So extensions can also be installed at the contract level.

Might might want to "enforce" only showing the supported ones, even though the colony has more installed.

rdig · 2022-07-27T22:53:07Z

src/modules/externalUrls.ts

-export const CM_LEARN_MORE = `https://colony.gitbook.io/colony/extensions/coin-machine`;
-export const CM_GET_WHITELISTED = `https://colony.gitbook.io/colony/get-started/how-to-get-whitelisted-for-the-clny-sale.`;
-export const CM_BLOG_POST = `https://blog.colony.io/introducing-coin-machine/`;
-export const CM_GOOGLE_SHEET = `https://docs.google.com/spreadsheets/d/1ZCuFcwqI4S6ZK5OwTl1yN7AK8mjv5d_V3g-_kMen01Y/edit#gid=2013814210`;
-export const CM_DESCRIPTION = 'https://colony.gitbook.io/colony/';


Maybe ask @arrenv or however is managing our Gitbook, if these links are still relevant to them, otherwise they should remove them on their end as well.

Good point, there is some content we will still want to use, so, I have temporarily added a note on each of the Coin Machine pages in GitBook until they can be removed.

rdig · 2022-07-27T22:54:35Z

src/utils/events/subgraphEvents.ts

@@ -260,7 +233,6 @@ export const parseSubgraphEvent = ({
      ...addressArgumentParser(parsedArguments),
      ...motionArgumentparser(parsedArguments),
      ...storageSlotArgumentParser(parsedArguments),
-      ...coinMachineEventsArgumentParser(parsedArguments),
      ...whitelistEventsArgumentParser(parsedArguments),


This one should be removed as well

chinins

The updates are all correct, so it's good to merge. I agree with Raul that this is a better approach.

chinins · 2022-07-29T13:06:44Z

src/data/staticData/extensionData.tsx

+type ExtensionDataPartialMap = Partial<
+  {
+    [E in Extension]: ExtensionData;
+  }


Very nice type casting!

danbr added the maintenance label Jul 21, 2022

danbr self-assigned this Jul 21, 2022

danbr requested a review from a team July 21, 2022 13:00

danbr marked this pull request as ready for review July 21, 2022 13:00

danbr mentioned this pull request Jul 21, 2022

Remove Whitelist & Coin Machine Extensions #3608

Closed

1 task

ArmandoGraterol suggested changes Jul 22, 2022

View reviewed changes

danbr requested a review from ArmandoGraterol July 25, 2022 06:47

willm30 requested changes Jul 25, 2022

View reviewed changes

ArmandoGraterol approved these changes Jul 25, 2022

View reviewed changes

willm30 mentioned this pull request Jul 25, 2022

Bug: App crashes when visiting the URL of an extension that doesn't exist #3678

Closed

willm30 self-requested a review July 25, 2022 18:38

willm30 approved these changes Jul 25, 2022

View reviewed changes

chinins approved these changes Jul 25, 2022

View reviewed changes

rdig approved these changes Jul 27, 2022

View reviewed changes

chinins approved these changes Jul 29, 2022

View reviewed changes

danbr added 6 commits August 1, 2022 13:34

refactor:remove coin machine & whitelist extensions

4819183

fix:remove WHITELIST_UPDATE

91ca7f8

fix:remove unused dependencies

06ee884

fix:remove more extension code, as per Olgas comments

3504717

fix:more removal of extension code

8c1838f

fix:refactor handling of dapp allowed extensions logic

94aa383

danbr force-pushed the refactor/3532-remove-whitelist-coinmachine branch from 2662619 to 94aa383 Compare August 1, 2022 10:38

danbr merged commit ac3d104 into master Aug 1, 2022

danbr deleted the refactor/3532-remove-whitelist-coinmachine branch August 1, 2022 11:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove coin machine & whitelist extensions #3663

Remove coin machine & whitelist extensions #3663

danbr commented Jul 21, 2022 •

edited

Loading

ArmandoGraterol left a comment

danbr commented Jul 25, 2022 •

edited

Loading

willm30 left a comment

ArmandoGraterol left a comment

ArmandoGraterol commented Jul 25, 2022

willm30 commented Jul 25, 2022

chinins left a comment

chinins Jul 25, 2022

chinins Jul 25, 2022

chinins Jul 25, 2022

chinins Jul 25, 2022

chinins Jul 25, 2022

chinins Jul 25, 2022

chinins Jul 25, 2022

willm30 commented Jul 26, 2022

danbr commented Jul 27, 2022

rdig left a comment

rdig Jul 27, 2022

rdig Jul 27, 2022

rdig Jul 27, 2022

rdig Jul 27, 2022

rdig Jul 27, 2022

rdig Jul 27, 2022

arrenv Jul 28, 2022 •

edited

Loading

rdig Jul 27, 2022

chinins left a comment

chinins Jul 29, 2022

		// @NOTE get list of installed extensions that are only allowed to be
		// used within the dapp

Remove coin machine & whitelist extensions #3663

Remove coin machine & whitelist extensions #3663

Conversation

danbr commented Jul 21, 2022 • edited Loading

Description

ArmandoGraterol left a comment

Choose a reason for hiding this comment

danbr commented Jul 25, 2022 • edited Loading

willm30 left a comment

Choose a reason for hiding this comment

ArmandoGraterol left a comment

Choose a reason for hiding this comment

ArmandoGraterol commented Jul 25, 2022

willm30 commented Jul 25, 2022

chinins 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

willm30 commented Jul 26, 2022

danbr commented Jul 27, 2022

rdig 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

arrenv Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbr commented Jul 21, 2022 •

edited

Loading

danbr commented Jul 25, 2022 •

edited

Loading

arrenv Jul 28, 2022 •

edited

Loading