Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Fix poa_getAllValidators endpoint #8700

Merged
merged 7 commits into from
Jul 26, 2023

Conversation

Phanco
Copy link
Contributor

@Phanco Phanco commented Jul 6, 2023

What was the problem?

This PR resolves #8698

How was it solved?

Fix the line of comparing address

How was it tested?

Test cases passed

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #8700 (bbd2052) into feature/6930-implement-poa-module (e5ee1a2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@                        Coverage Diff                         @@
##           feature/6930-implement-poa-module    #8700   +/-   ##
==================================================================
  Coverage                              83.59%   83.59%           
==================================================================
  Files                                    610      610           
  Lines                                  22665    22667    +2     
  Branches                                3289     3289           
==================================================================
+ Hits                                   18946    18948    +2     
  Misses                                  3719     3719           
Impacted Files Coverage Δ
framework/src/modules/poa/endpoint.ts 93.61% <100.00%> (+0.13%) ⬆️
framework/src/modules/poa/module.ts 96.94% <100.00%> (+0.02%) ⬆️

@Phanco Phanco changed the base branch from development to feature/6930-implement-poa-module July 7, 2023 08:56
@Phanco Phanco requested review from mitsuaki-u and sitetester July 7, 2023 09:02
@Phanco Phanco marked this pull request as ready for review July 7, 2023 09:12
@ishantiw ishantiw requested review from ishantiw and removed request for mitsuaki-u July 7, 2023 09:25
@sitetester sitetester changed the title Fix poa_getAllValidators Fix poa_getAllValidators endpoint Jul 7, 2023
@sitetester
Copy link
Contributor

sitetester commented Jul 7, 2023

  • Data from getAllValidators endpoint is returning in unexpected sorting order on next execution. This can presumably cause build failure in future.
  • Can modify like this to apply sorting
response.sort((v1, v2) => v1.name.localeCompare(v2.name, 'en'));
return { validators: response };
  • Need one more test to differentiate between active & non active validator weight
it('should return json with empty weight for non active validator', async () => {
	await validatorStore.set(createStoreGetter(stateStore), address1, { name: 'validator1'});
	await validatorStore.set(createStoreGetter(stateStore), address2, { name: 'validator2' });
	const currentSnapshot = {
		threshold: BigInt(2),
		validators: [
			{
				address: address1,
				weight: BigInt(1),
			},
		],
	};
	await snapshotStore.set(createStoreGetter(stateStore), KEY_SNAPSHOT_0, currentSnapshot);

	const { validators: validatorsDataReturned } = await poaEndpoint.getAllValidators(
		createTransientModuleEndpointContext({ stateStore }),
	);

	expect(validatorsDataReturned[0].weight).toBe(currentSnapshot.validators[0].weight.toString());
	expect(validatorsDataReturned[1].weight).toBe('');
});

So the tests are passing anyway as both validators have same weight 1. If we change one of the weights, it will fail on next iteration.
Screenshot 2023-07-10 at 04 17 40 PM

Screenshot 2023-07-10 at 04 18 56 PM - Also please add this comment above these lines
// Here we are checking against name sorted values
expect(validatorsDataReturned[0].weight).toBe(snapshot.validators[0].weight.toString());
expect(validatorsDataReturned[1].weight).toBe(snapshot.validators[1].weight.toString());

So 3 changes can be done in this PR

  • Changing return type from ValidatorEndpoint to Validator
  • Changing address2 weight to 2, here
const snapshot = {
		threshold: BigInt(2),
		validators: [
			{
				address: address1,
				weight: BigInt(1),
			},
			{
				address: address2,
				weight: BigInt(2), // ***
			},
		],
	};

& then removing unnecessary validatorsDataReturned from const { validators: validatorsDataReturned }

Copy link
Contributor

@sitetester sitetester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feedback submitted

@Phanco Phanco requested a review from sitetester July 10, 2023 13:52
Copy link
Contributor

@sitetester sitetester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment updated with some more requested changes

@sitetester sitetester assigned sitetester and unassigned Phanco Jul 21, 2023
@sitetester sitetester requested a review from mitsuaki-u July 21, 2023 15:24
@shuse2 shuse2 requested a review from sitetester July 26, 2023 06:17
@sitetester sitetester removed the request for review from mitsuaki-u July 26, 2023 07:30
@shuse2 shuse2 enabled auto-merge (squash) July 26, 2023 09:17
@shuse2 shuse2 merged commit 372bae0 into feature/6930-implement-poa-module Jul 26, 2023
@shuse2 shuse2 deleted the 8698-fix-getAllValidators branch July 26, 2023 09:18
ishantiw pushed a commit that referenced this pull request Jul 31, 2023
* Update getAllValidators Endpoint

* Update test cases for poa_getAllValidators

* Sort response by name on getAllValidators function

* Update PoA test cases

* Update code per PR comments

---------

Co-authored-by: Khalid Hameed <sitetester2010@gmail.com>
ishantiw added a commit that referenced this pull request Aug 1, 2023
* Bootstrap PoA files and folders (#8456)

* Bootstrap PoA files and folders

* Update method names on module.ts

* Replace .keep to index.ts for folders

* Add bootstrap test file

* Rename internal method to PoAInternalMethod

* Implement events for PoA (#8464)

* Bootstrap PoA files and folders

* Update method names on module.ts

* Replace .keep to index.ts for folders

* Add bootstrap test file

* Create events and register of the said event

* Update enum name

* Add stores to PoA module (#8466)

* 🌱 Initialize stores

* ♻️  Update names and register stores

* 💅 Rename snapshotStoreSchema->snapshotSchema

* Define genesis PoA store schema & relevant types (#8501)

* Define genesis PoA store schema & relevant types

* `length' replaced with `minLength` & `maxLength`

* Add missing $id

* PoA Register Authority Command (#8496)

* Implement Register Authority Commands

* Renaming Commands

* Remove extra files

* Added Unit Test for RegisterAuthority

* Put types back to PoA

* Update code according to PR review and comments

* Update chainID in test case to match with actual format

* Update param names to be grammar-correct

* Update AUTHORITY_REGISTRATION_FEE comments and test unit imports

* Update $id for updateAuthority Schema

* Change name of updateAuthoritySchema

* Implement afterTransactionsExecute and _shuffleValidatorsList (#8523)

* 🌱 Implement afterTransactionsExecute and _shuffleValidatorsList

* ✅ Add unit tests for afterTransactionsExecute and shuffleValidatorList

* ♻️ Use constants and simplify shuffleValidatorList

* ✅ Adjust expectation setValidatorsParams to expect secondSnapshot threshold

* 💅🏻 Rename snapshot keys

* ♻️ Add missing logic and improve logic for afterTransactionsExecute

* 💅🏻 Assert type on the arguments for readability

* ♻️Naming issues and refine loop in test

* Implement update authority command (#8527)

* Implement Register Authority Commands

* Renaming Commands

* Remove extra files

* Added Unit Test for RegisterAuthority

* Put types back to PoA

* Update code according to PR review and comments

* Update chainID in test case to match with actual format

* Update param names to be grammar-correct

* Update AUTHORITY_REGISTRATION_FEE comments and test unit imports

* Update $id for updateAuthority Schema

* Implement Update Authority and Test Cases

* Change name of updateAuthoritySchema

* Update updateAuthoritySchema naming

* Update update authority coding according to PR comments

* Edited Buffer bits and test cases captions

* Update cosmetic change on update_authority files

* Add comments and better error message to UpdateAuthorityCommand

* Minor changes to error message captions

* Update error message caption

* Implement PoA Update Generator Key (#8534)

* Implement Register Authority Commands

* Renaming Commands

* Remove extra files

* Added Unit Test for RegisterAuthority

* Put types back to PoA

* Update code according to PR review and comments

* Update chainID in test case to match with actual format

* Update param names to be grammar-correct

* Update AUTHORITY_REGISTRATION_FEE comments and test unit imports

* Update $id for updateAuthority Schema

* Implement Update Authority and Test Cases

* Change name of updateAuthoritySchema

* Implement UpdateGeneratorKey and Test Cases

* Update updateAuthoritySchema naming

* Update update authority coding according to PR comments

* Edited Buffer bits and test cases captions

* Update cosmetic change on update_authority files

* Update UpdateGeneratorKeyCommand to use senderAddress

* Add comments and better error message to UpdateAuthorityCommand

* Minor changes to error message captions

* Update error message caption

---------

Co-authored-by: !shan <ishantiw.quasar@gmail.com>

* Implement proof of authority genesis initialization (#8525)

* Add PoA constants, schemas and types

* Implement proof of authority genesis initialization

* Update error messages in poa module initGenesis

* Export types and fix snapshotstore type

* Implement tests for poa genesis initialization

* Remove unused validators from test

* Update snapshot storage

* Implement finalizeGenesisState

* Update lint no-console place holder

---------

Co-authored-by: Mitsuaki Uchimoto <mitsuaki-u@users.noreply.github.com>

* Implement PoA module endpoints (#8581)

* Implement PoA module endpoints

* Add getRegistrationFee endpoint

---------

Co-authored-by: Mitsuaki Uchimoto <mitsuaki-u@users.noreply.github.com>

* Resolve PoA dependencies (#8584)

* ♻️  Resolve dependencies and add configurable constant

* ♻️  Use authorityRegistrationFee in endpoint and fix test

* Add PoAMethod to src/index.ts

* Use PoA dependency

* Move setValidatorsParams to under registerValidatorKeys (#8672)

Co-authored-by: !shan <ishantiw.quasar@gmail.com>

* Expose PoA Commands (#8699)

Expose Commands on PoA

* PoA Example (And update to application.ts) (#8663)

* Add PoA example

* Update PoA Genesis

* Update README and remove uncessary genesis json

* Update application.ts to accept PoS and PoA

* Add Reward Module to PoA

* Renaming poa to sidechain

* Revert application.ts

* Move setValidatorsParams to under registerValidatorKeys

* Register module inside the PoA

* Add RewardModule and revert module.ts (Changes in separate PR)

* Add sign scripts for updateAuthority.ts

* Update yarn.lock

* Fix `registerValidatorKeys` (#8703)

Co-authored-by: !shan <ishantiw.quasar@gmail.com>

* Fix `poa_getAllValidators` endpoint (#8700)

* Update getAllValidators Endpoint

* Update test cases for poa_getAllValidators

* Sort response by name on getAllValidators function

* Update PoA test cases

* Update code per PR comments

---------

Co-authored-by: Khalid Hameed <sitetester2010@gmail.com>

* ♻️  Update interfaces used in PoA

* ✅ Fix test and lint error

---------

Co-authored-by: Franco NG <franco.ng@lightcurve.io>
Co-authored-by: sitetester <sitetester2010@gmail.com>
Co-authored-by: Mitsuaki Uchimoto <36514357+mitsuaki-u@users.noreply.github.com>
Co-authored-by: Mitsuaki Uchimoto <mitsuaki-u@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Endpoint poa_getAllValidators did not return active validators' weight
4 participants