Define Angular Services for Voluntary Exit #164

olehyev138 · 2021-03-10T01:46:23Z

Created Account voluntary exit page.

On the Accounts action added a new button for navigating to the account voluntary exit page.

On the Account table, there is another action button to navigating to the account voluntary exit page for exiting the selected key.

The Account voluntary exit would require to select the keys for exit and a conformation for exit

rkapka · 2021-03-10T08:08:37Z

src/app/modules/core/services/wallet.service.ts

@@ -94,4 +94,8 @@ export class WalletService {
  recover(request: RecoverWalletRequest): Observable<any> {
    return this.http.post(`${this.apiUrl}/wallet/recover`, request);
  }
+
+  exitAccounts(request: AccountVoluntaryExitRequest): Observable<any> {
+    return this.http.post(`${this.apiUrl}/wallet/exit-accoints`, request);


typo: accoints -> accounts

Hey @olehyev138 I think this endpoint will be ${this.apiUrl}/accounts/exit when I implement it in the backend

I will update the code

rkapka · 2021-03-10T08:09:00Z

src/app/modules/onboarding/validators/utility.validator.ts

+    };
+  }
+
+  static MustHaveMoreThanOneControle(grp: FormGroup): ValidationErrors | null {


typo: Controle -> Control

Also, I will update this also

rkapka · 2021-03-10T08:11:44Z

src/app/modules/wallet/pages/account-voluntary-exit/account-voluntary-exit.component.html

+
+                        <span
+                            *ngIf="exitAccountFormGroup?.get('confirmation')?.hasError('incorectValue')">
+                            You must type agree


Let's also have 'agree' here for better readability.

Confirmation is required. You must type 'agree'

On https://deploy-preview-164--prysm-web-ui.netlify.app/dashboard/wallet/accounts/voluntary-exit the text below the input does not fit on one line, which does not look too good. I assume this is because the control's width is too small.

I will fix this

rkapka · 2021-03-10T08:14:33Z

src/app/proto/validator/accounts/v2/web_api.ts

+  /**
+   * Confirmation if the exit should proceed or not
+   */
+  confrimation: string;


I don't think confrimation is needed at all. Once we submit the request, it will always be true (it doesn't make sense to submit a request when it's false).

I will remove it from the code i thought maybe you would double-check on the back end.

rkapka · 2021-03-10T08:25:39Z

src/app/modules/wallet/pages/account-voluntary-exit/account-voluntary-exit.component.html

+                    Are you sure you want exit these account?
+
+                    Once these accounts are exited, the action cannot be
+                    reverted.


I think a more suitable wording would be

Please make sure you understand the consequences of performing a voluntary exit. Once an account is exited, the action cannot be reverted

Also add in another sentence: "You will not be able to withdraw your ETH if exited until ETH1 is merged with ETH2, which could happen in over a year"

rkapka · 2021-03-10T08:28:12Z

src/app/modules/wallet/pages/account-voluntary-exit/account-voluntary-exit.component.ts

+  exitAccountFormGroup: FormGroup | undefined | null;
+  keys: Account[] = [];
+  ngOnInit(): void {
+    const publicKey = this.activatedRoute.snapshot.queryParams.publicKey;


We don't have a publicKey query parameter when exiting accounts.

Is the idea here that we will use a single public key in the route query parameters to perform the API request? If a user is trying to exit 100 keys at once that might be too big for the route. Is there another way we can pass in a list of public keys we want to exit into this component without using browser query params?

This will always be one public key since this navigation is through the grid.
I don't think there is a way to pass the keys to this page any other way since we are navigating to that page.
But we could store them in the service and get them from there

rkapka · 2021-03-10T08:34:08Z

src/app/modules/wallet/pages/account-voluntary-exit/account-voluntary-exit.component.ts

+    this.walletService
+      .accounts()
+      .pipe(map((x) => this.searchbyPublicKey(publicKey, x.accounts)))
+      .subscribe((accs) => {
+        accs.forEach((acc) => {
+          this.keys.push(acc);
+        });
+      });
+  }


We should display all keys here, not search by a specific public key.

This is also related to the previous comment if you want i can remove that feature

rkapka · 2021-03-10T08:35:11Z

src/app/modules/wallet/pages/account-voluntary-exit/account-voluntary-exit.component.ts

+    }
+    const request = {
+      publicKeys: this.keys.map((x) => x.validatingPublicKey),
+      confrimation: this.exitAccountFormGroup?.get('confirmatio')?.value,


typo: confirmatio -> confirmation

rkapka · 2021-03-10T08:39:14Z

src/app/modules/wallet/pages/account-voluntary-exit/account-voluntary-exit.component.ts

+      const removedKeys =
+        Object.keys(this.exitAccountFormGroup?.controls ?? {}).length - 1;
+      this.snackMsgService.open(
+        `Successfully exited from ${removedKeys} keys`,


Should be

Successfully exited ${removedKeys} key(s)

rkapka · 2021-03-10T08:41:41Z

src/app/modules/wallet/pages/account-voluntary-exit/account-voluntary-exit.component.ts

+    } as AccountVoluntaryExitRequest;
+
+    this.walletService.exitAccounts(request).subscribe((x) => {
+      const removedKeys =


Can we change the name of the variable to exitedKeys to use the same naming everywhere? Removing/deleteing keys is a different action.

rauljordan · 2021-03-10T14:48:56Z

src/app/modules/core/services/wallet.service.ts

@@ -94,4 +94,8 @@ export class WalletService {
  recover(request: RecoverWalletRequest): Observable<any> {
    return this.http.post(`${this.apiUrl}/wallet/recover`, request);
  }
+
+  exitAccounts(request: AccountVoluntaryExitRequest): Observable<any> {
+    return this.http.post(`${this.apiUrl}/wallet/exit-accoints`, request);


Hey @olehyev138 I think this endpoint will be ${this.apiUrl}/accounts/exit when I implement it in the backend

rauljordan · 2021-03-10T14:49:30Z

src/app/modules/shared/components/base.component.ts

  ngOnDestroy(): void {
    this.destroyed$.next();
    this.destroyed$.complete();
  }
+
+  back(): void {
+    history.back();


Curious, where is history defined? is it a browser global?

rauljordan · 2021-03-10T14:51:02Z

src/app/modules/wallet/pages/account-voluntary-exit/account-voluntary-exit.component.html

+                    Are you sure you want exit these account?
+
+                    Once these accounts are exited, the action cannot be
+                    reverted.


Also add in another sentence: "You will not be able to withdraw your ETH if exited until ETH1 is merged with ETH2, which could happen in over a year"

rauljordan · 2021-03-10T14:53:34Z

src/app/modules/wallet/pages/account-voluntary-exit/account-voluntary-exit.component.ts

+  exitAccountFormGroup: FormGroup | undefined | null;
+  keys: Account[] = [];
+  ngOnInit(): void {
+    const publicKey = this.activatedRoute.snapshot.queryParams.publicKey;


Is the idea here that we will use a single public key in the route query parameters to perform the API request? If a user is trying to exit 100 keys at once that might be too big for the route. Is there another way we can pass in a list of public keys we want to exit into this component without using browser query params?

rauljordan · 2021-03-10T14:54:40Z

src/app/modules/wallet/pages/account-voluntary-exit/account-voluntary-exit.component.ts

+      } as AbstractControlOptions
+    );
+
+    this.walletService


It's possible we want to exit N public keys where N is anything from 1 to the total number of keys in the wallet, inclusive. Maybe we can input a list of public keys into this component and then you wont need to query the wallet service

rauljordan · 2021-03-10T14:55:21Z

src/app/proto/validator/accounts/v2/web_api.ts

+  /**
+   * Confirmation if the exit should proceed or not
+   */
+  confrimation: string;


Typo here: confirmation

rauljordan · 2021-03-10T19:17:35Z

src/app/modules/core/mocks/index.ts

@@ -116,6 +116,7 @@ export const generateBalancesForEpoch = (url: string) => {

 export const Mocks: IMocks = {
  '/v2/validator/wallet/recover': {},
+  '/v2/validator/wallet/account/exit': {},


Suggested change

'/v2/validator/wallet/account/exit': {},

'/v2/validator/wallet/accounts/exit': {},

rauljordan · 2021-03-10T19:38:20Z

src/app/proto/validator/accounts/v2/web_api.ts

+   * The key from the accounts that will be removed
+   */
+  publicKeys: string[];
+


Suggested change

rauljordan · 2021-03-10T19:45:22Z

We should probably make import keystores primary color and exit keystores the secondary color above and should have a little bit more spacing between each other.

For some reason, I also can't click on the import keystores button for some reason, it no longer works and does not take me to the import accounts form.

The form above has a rough UI. There is no title, the padding is weird and perhaps too little, and there is no alignment of elements correctly (proper left justification). It is also not clear that the account is selectable until you however over it. We should be displaying the first 12 or so characters of each public key instead of the name. For example 0x29392323... instead of its name like "merely simple gator". What will this form look like if a user has 100 or more keys, for example? The UI should account for this. For reference of a properly aligned UI, with a title and subtitle text, I would recommend checking the import accounts form in the screenshot below:

rauljordan · 2021-03-10T19:45:46Z

We should also have a way to "select all" accounts to exit

rauljordan · 2021-03-10T22:17:41Z

This spacing, padding, and layout is still weird. Can we just match the style from the import accounts form? The same paddings, the same style of title (import accounts is not bold)? Check out the screenshot of the import keystores form I added in my previous comment, the paddings are not the same here

Also, what if a user has 100 or 1000 keys, will the form become extremely long or is there anything we can do with making the list selection scrollable?

olehyev138 · 2021-03-10T22:53:46Z

Also, what if a user has 100 or 1000 keys, will the form become extremely long or is there anything we can do with making the list selection scrollable?

The list is encapsulated by a virtual scroller i thought that you would have noticed that.
So there won't be issues with a long list

rauljordan

Great, thanks for doing this! Needs approve from @rkapka for us to be able to merge

rkapka

Just two more comments.

rkapka · 2021-03-11T08:48:05Z

src/app/modules/wallet/pages/account-voluntary-exit/account-voluntary-exit.component.ts

+      this.snackMsgService.open(
+        `Successfully exited ${exitedKeys} key(s)`,
+        'Success',
+        {
+          direction: 'rtl',
+          politeness: 'assertive',
+          duration: 4000,
+        }
+      );


Can we make this snackbar and the below snackbar consistent with each other, as they both indicate a successful operation? Maybe define some utility service to make the code DRY?

prysm-web-ui/src/app/modules/wallet/pages/import/import.component.ts

Lines 49 to 51 in 1d44a70

this.snackBar.open('Successfully imported keystores', 'Close', {

duration: 4000,

});

rkapka · 2021-03-11T08:48:23Z

src/app/modules/wallet/pages/account-voluntary-exit/account-voluntary-exit.component.html

+
+                            <span
+                                *ngIf="exitAccountFormGroup?.get('confirmation')?.hasError('incorectValue')">
+                                You must type agree


Suggested change

You must type agree

You must type 'agree'

ad Voluntary Exit feature

71d032d

rkapka requested changes Mar 10, 2021

View reviewed changes

rkapka mentioned this pull request Mar 10, 2021

Lazy Load X Modules #165

Merged

rauljordan reviewed Mar 10, 2021

View reviewed changes

PR issue fix

8ac95b4

rauljordan reviewed Mar 10, 2021

View reviewed changes

olehyev138 added 2 commits March 10, 2021 16:07

style update

c919232

lint issue fix and selet all option added

1ae9e7c

style-update

51e32d8

rauljordan approved these changes Mar 10, 2021

View reviewed changes

rkapka requested changes Mar 11, 2021

View reviewed changes

Notification service created

1760f4b

rkapka approved these changes Mar 12, 2021

View reviewed changes

rkapka merged commit 5448989 into prysmaticlabs:master Mar 12, 2021

olehyev138 deleted the feature/voluntary-exit branch March 15, 2021 18:16

	'/v2/validator/wallet/account/exit': {},
	'/v2/validator/wallet/accounts/exit': {},

	this.snackBar.open('Successfully imported keystores', 'Close', {
	duration: 4000,
	});

Define Angular Services for Voluntary Exit #164

Define Angular Services for Voluntary Exit #164

Conversation

olehyev138 commented Mar 10, 2021

rkapka Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka Mar 10, 2021 • edited Loading

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

rkapka Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

rkapka Mar 10, 2021 • edited Loading

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

rauljordan commented Mar 10, 2021

rauljordan commented Mar 10, 2021

rauljordan commented Mar 10, 2021

olehyev138 commented Mar 10, 2021

rauljordan left a comment

Choose a reason for hiding this comment

rkapka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka Mar 10, 2021 •

edited

Loading

rkapka Mar 10, 2021 •

edited

Loading

rkapka Mar 10, 2021 •

edited

Loading

rkapka Mar 10, 2021 •

edited

Loading

rkapka Mar 10, 2021 •

edited

Loading