Drop all dependencies on marketplace and wallet #666

odysseus654 · 2020-08-30T23:05:22Z

This PR attempts to drop all references to the High Fidelity marketplace and wallet. Some effort has been made to prevent this from being a protocol-break (although there's a fair number of enums and entity packet content that could be pruned out once this is landed).

Going over the commits, things this thing removes:

All avatar and domain ownership challenges
Queries to FST files to see if they came from the marketplace
Permissions "can rez certified" and "can temprez certified"
The "Wallet" and "Marketplace" apps, along with the majority of the "Security" app
A context overlay of some kind showing the current marketplace status of any item
The "upload Avatar to Marketplace" tool
The scripting objects ContextOverlay and WalletScriptingInterface
12 entity properties exclusively used for the marketplace (still kept in the protocol stream for compatibility, but unused)
Assignment Clients being paid for their work (related: the -wallet option on assignment-client)

Note that I did pick up on an apparent typo in AssignmentClientApp that on its surface appears to effectively disable the -a and -p options; I'm sorely tempted to break this up into its own patch for independent review (it must stay as part of this patch as well though)

two-one-five · 2020-08-30T23:35:55Z

ContextOverlay may or may not have been useful for security reasons? We should look into that to ensure there's not some usefulness to having something like that regardless.

odysseus654 · 2020-08-30T23:40:55Z

Yah that was a bit confusing to me too. Everything I saw had it keyed to ownership challenges for objects though, which was square in the crosshairs of this PR. Once I dropped those there was basically nothing left. I don't really know the value of empty containers at this point. Heck I think I may have left an empty .cpp file in this project after stripping out all the code...

odysseus654 · 2020-08-30T23:50:26Z

MixerAvatar.cpp likely can be deleted as well. MixerAvatar.h is still in use and appears to have value (even though 90% of it has been removed)

vircadia-build-notifier · 2020-08-31T02:18:16Z

The following links are available:
build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR666/Vircadia-Alpha-PR666-0ddb89a_c26bec3a.exe

build (macOS-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR666/Vircadia-Alpha-PR666-0ddb89a_c26bec3a.dmg

build (ubuntu-18.04, full)

daleglass

Well, this is a lot to review, but given that it's mostly deletion if that goes wrong it should just fail to compile.

The actual changes made make sense to me though I do have a few nitpicks.

daleglass · 2020-09-06T22:13:18Z

assignment-client/src/avatars/MixerAvatar.h

@@ -21,73 +21,24 @@ class ResourceRequest;
 class MixerAvatar : public AvatarData {
    Q_OBJECT
 public:
-    MixerAvatar();
+    inline MixerAvatar() {}


Why this change?

The entire contents of the MixerAvatar() constructor has been removed (and the .cpp file can likely be deleted as it's pretty empty). At this point it's an empty constructor. Alternative suggestions?

Mm, fair enough

daleglass · 2020-09-06T22:22:24Z

libraries/entities/src/EntityItem.cpp

+        APPEND_ENTITY_PROPERTY(PROP_ITEM_ARTIST, QString());
+        APPEND_ENTITY_PROPERTY(PROP_ITEM_LICENSE, QString());
+        APPEND_ENTITY_PROPERTY(PROP_LIMITED_RUN, quint32(-1));
+        APPEND_ENTITY_PROPERTY(PROP_MARKETPLACE_ID, QString());


Maybe remove this one and those below? Name, description, categories, artist, license are pretty much certain to come handy in the future, but the rest look like hifi marketplace specific values.

These properties are specific to the marketplace. I've done this so as to not break protocol until we're ready to, but if we keep them in this PR a later PR would likely drop these lines.

daleglass · 2020-09-06T22:22:43Z

libraries/entities/src/EntityItem.cpp

+    SKIP_ENTITY_PROPERTY(PROP_ITEM_ARTIST, QString);
+    SKIP_ENTITY_PROPERTY(PROP_ITEM_LICENSE, QString);
+    SKIP_ENTITY_PROPERTY(PROP_LIMITED_RUN, quint32);
+    SKIP_ENTITY_PROPERTY(PROP_MARKETPLACE_ID, QString);


These properties are specific to the marketplace. I've done this so as to not break protocol until we're ready to, but if we keep them in this PR a later PR would likely drop these lines.

odysseus654 · 2020-09-06T23:47:46Z

Have rebased & squished the commits in this PR. Will compile/smoketest as well as remove MixerAvatar.cpp

odysseus654 · 2020-09-07T00:07:09Z

Smoketested the interface and dropped the empty MixerAvatar.cpp file. Note that I have not yet done any testing of the server.

vircadia-build-notifier · 2020-09-07T02:23:32Z

The following links are available:
build (ubuntu-18.04, full)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR666/Vircadia-Alpha-PR666-f91e841_b9dbda9c.exe

build (macOS-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR666/Vircadia-Alpha-PR666-f91e841_b9dbda9c.dmg

JulianGro · 2020-09-23T16:36:57Z

Could you get rid of the merge conflicts so I can build and test this?
I cannot view the conflict, so I have no idea how big of a change it might be.

odysseus654 · 2020-10-04T03:29:44Z

Rebased / squished the commits in this PR. Have compiled / smoketested to check for any obvious issues.

vircadia-build-notifier · 2020-10-04T05:00:14Z

The following links are available:
build (macOS-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR666/Vircadia-Alpha-PR666-5535bf8_3a4051bd.dmg

build (ubuntu-18.04, full)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/kasenvr_project-athena/PR666/Vircadia-Alpha-PR666-5535bf8_3a4051bd.exe

JulianGro · 2020-10-04T12:17:51Z

Building works fine at the very least https://appimage.moto9000.moe/experimental/PR666/Vircadia-x86_64_PR666_3a4051b.AppImage

JulianGro · 2020-10-08T22:33:04Z

I haven't had any problems with this yet.

ctrlaltdavid · 2020-10-09T21:32:55Z

I think it would be better, as a first step, to disable items (e.g., don't show UI and/or comment out code) rather than a wholesale deletion of code - some of which may prove useful in the future (either as actual code or implementation hints) should we want to address similar functionality. Some code might be able to be removed after considering it, piece by piece.

odysseus654 · 2020-10-13T02:13:47Z

This deletion approach was done as it tends to be a bright-line describing all the code in the system that depends on the wallet in some form. Commenting out code can be a fine line between keeping code that may be useful in the system and keeping significant junk in the codebase that will never be used in the future. I do recognize the amount of loss that is involved in this patch though; a lot of the deletions I've made represented unrealized potential that I would really have liked to have seen happen.

If some of this code has value to be kept in the system then yah let's keep it in (potentially commented out). Such decisions would have to be intentional though; the code is linked together enough that cauterizing parts of it out would take a bit of effort.

I would not be against somehow breaking this PR up into smaller sub-PRs (and smaller decisions on what to drop).

ctrlaltdavid · 2020-10-15T18:56:15Z

A series of smaller PRs, each focused on a particular area, may well be better. Even if, combined, they all end up removing the same code, at least we'll have merge commits in effect documenting code points likely to be involved if we want to re-enable or implement some particular feature at some later date.

odysseus654 · 2020-10-15T19:43:24Z

No major objections, PR #667 was originally part of this patch too. Would like to keep this PR open until a final decision is made on the whole (or the remaining) though.

odysseus654 · 2020-10-15T19:44:16Z

Although breaking this up into smaller pieces would likely cause some extra work to be needed depending on the amount of effort required to cauterize the linkages. I wouldn't mind suggestions.

daleglass · 2021-01-16T19:25:59Z

Dev meeting: it could be interesting to review why the merge is failing here -- this is code to remove, so why are changes being made that affect it?

vircadia-build-notifier · 2021-04-25T05:33:44Z

The following links are available:
build (macOS-latest, client)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR666/Vircadia-Interface-PR666-f0e4ca0-_3dbaaf9e.dmg

build (macOS-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR666/Vircadia-PR666-f0e4ca0-_3dbaaf9e.dmg

build (ubuntu-18.04, full)

build (ubuntu-18.04, android)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR666/Vircadia-PR666-f0e4ca0-_3dbaaf9e.exe

vircadia-build-notifier · 2021-08-25T05:58:30Z

The following links are available:
build (macOS-latest, client)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR666/Vircadia-Interface-PR666-bd481d1-_97d2dc56.dmg

build (ubuntu-18.04, full)

build (macOS-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR666/Vircadia-PR666-bd481d1-_97d2dc56.dmg

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR666/Vircadia-PR666-bd481d1-_97d2dc56.exe

Penguin-Guru · 2021-12-02T17:42:32Z

Whether it's broken up for removal or not, I think it's good to remove since it's not being used. If you think it might be useful to implement something in the future, you could then open a new PR adding those parts back in. That might be more subject to merge issues but I think there is practical value in streamlining the code base so it is more intuitive to new contributors. Hopefully this would also speed up the build process a bit.

I think there has been some talk of implementing hypothetical market/commerce functionality purely as a tablet application, or perhaps using a server-side plug-in or module, rather than building it into the application code. Correct me if I'm wrong.

vircadia-build-notifier · 2022-01-26T15:36:52Z

The following links are available:
build (ubuntu-18.04, full)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR666/Vircadia-PR666-e0b209c-_822a0982.exe

build (macOS-10.15, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR666/Vircadia-PR666-e0b209c-_822a0982.dmg

build (macOS-10.15, client)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR666/Vircadia-Interface-PR666-e0b209c-_822a0982.dmg

stale · 2022-07-26T00:52:57Z

Hello! Is this still an issue?

two-one-five added allow-build-upload Allows the upload of a build for non white-listed users rebuild rebuild through the GithubActions labels Aug 31, 2020

two-one-five added the scripting api change Change is made in the scripting API label Aug 31, 2020

ctrlaltdavid added the hifi migration Issues and PRs relating to migrating away from High Fidelity dependency. label Sep 3, 2020

daleglass mentioned this pull request Sep 3, 2020

It seems the avatar you are wearing is registered to another user. #635

Closed

daleglass suggested changes Sep 6, 2020

View reviewed changes

daleglass approved these changes Sep 6, 2020

View reviewed changes

odysseus654 force-pushed the pr/drop-wallet branch from c26bec3 to 14bf816 Compare September 6, 2020 23:46

JulianGro mentioned this pull request Sep 8, 2020

Assignment Client CLI will need update vircadia/vircadia-docs-sphinx#77

Closed

daleglass mentioned this pull request Sep 12, 2020

Developer menu still has "Marketplace Item Tester" #679

Closed

two-one-five added do not merge do not merge due to issues or pending updates CR Approved At least one code reviewer has approved the PR. labels Sep 13, 2020

odysseus654 force-pushed the pr/drop-wallet branch from b9dbda9 to 3a4051b Compare October 4, 2020 03:28

odysseus654 force-pushed the pr/drop-wallet branch from 3a4051b to 3dbaaf9 Compare April 25, 2021 03:42

odysseus654 force-pushed the pr/drop-wallet branch from 3dbaaf9 to 97d2dc5 Compare August 25, 2021 04:32

digisomni added the dormant This PR is on hold but has interest/use surrounding it. label Nov 6, 2021

stripping wallet/marketplace out

822a098

odysseus654 force-pushed the pr/drop-wallet branch from 97d2dc5 to 822a098 Compare January 25, 2022 06:46

stale bot added the stale Issue / PR has not had activity label Jul 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop all dependencies on marketplace and wallet #666

Drop all dependencies on marketplace and wallet #666

odysseus654 commented Aug 30, 2020

two-one-five commented Aug 30, 2020

odysseus654 commented Aug 30, 2020

odysseus654 commented Aug 30, 2020

vircadia-build-notifier commented Aug 31, 2020

daleglass left a comment

daleglass Sep 6, 2020

odysseus654 Sep 6, 2020

daleglass Sep 6, 2020

daleglass Sep 6, 2020

odysseus654 Sep 6, 2020

daleglass Sep 6, 2020

odysseus654 Sep 6, 2020

odysseus654 commented Sep 6, 2020

odysseus654 commented Sep 7, 2020

vircadia-build-notifier commented Sep 7, 2020

JulianGro commented Sep 23, 2020

odysseus654 commented Oct 4, 2020

vircadia-build-notifier commented Oct 4, 2020

JulianGro commented Oct 4, 2020

JulianGro commented Oct 8, 2020

ctrlaltdavid commented Oct 9, 2020

odysseus654 commented Oct 13, 2020

ctrlaltdavid commented Oct 15, 2020

odysseus654 commented Oct 15, 2020

odysseus654 commented Oct 15, 2020

daleglass commented Jan 16, 2021

vircadia-build-notifier commented Apr 25, 2021

vircadia-build-notifier commented Aug 25, 2021

Penguin-Guru commented Dec 2, 2021

vircadia-build-notifier commented Jan 26, 2022

stale bot commented Jul 26, 2022

Drop all dependencies on marketplace and wallet #666

Are you sure you want to change the base?

Drop all dependencies on marketplace and wallet #666

Conversation

odysseus654 commented Aug 30, 2020

two-one-five commented Aug 30, 2020

odysseus654 commented Aug 30, 2020

odysseus654 commented Aug 30, 2020

vircadia-build-notifier commented Aug 31, 2020

daleglass left a comment

Choose a reason for hiding this comment

daleglass Sep 6, 2020

Choose a reason for hiding this comment

odysseus654 Sep 6, 2020

Choose a reason for hiding this comment

daleglass Sep 6, 2020

Choose a reason for hiding this comment

daleglass Sep 6, 2020

Choose a reason for hiding this comment

odysseus654 Sep 6, 2020

Choose a reason for hiding this comment

daleglass Sep 6, 2020

Choose a reason for hiding this comment

odysseus654 Sep 6, 2020

Choose a reason for hiding this comment

odysseus654 commented Sep 6, 2020

odysseus654 commented Sep 7, 2020

vircadia-build-notifier commented Sep 7, 2020

JulianGro commented Sep 23, 2020

odysseus654 commented Oct 4, 2020

vircadia-build-notifier commented Oct 4, 2020

JulianGro commented Oct 4, 2020

JulianGro commented Oct 8, 2020

ctrlaltdavid commented Oct 9, 2020

odysseus654 commented Oct 13, 2020

ctrlaltdavid commented Oct 15, 2020

odysseus654 commented Oct 15, 2020

odysseus654 commented Oct 15, 2020

daleglass commented Jan 16, 2021

vircadia-build-notifier commented Apr 25, 2021

vircadia-build-notifier commented Aug 25, 2021

Penguin-Guru commented Dec 2, 2021

vircadia-build-notifier commented Jan 26, 2022

stale bot commented Jul 26, 2022