Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated to read the balance of veNation for the Passport Holder and filter when less than 1.5 #4

Merged
merged 8 commits into from
Nov 25, 2023

Conversation

johnmark13
Copy link

What do you think? We need to have some passport holders with < 1.5 to test with, but from the manual testing I have done I believe that this works.

@johnmark13 johnmark13 changed the title Largely untested, but passes unit tests Updated to read the balance of veNation for the Passport Holder and filter when less than 1.5 Oct 3, 2023
@johnmark13 johnmark13 marked this pull request as ready for review October 3, 2023 07:48
Copy link
Member

@aahna-ashina aahna-ashina left a comment

Choose a reason for hiding this comment

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

@johnmark13 Not up-to-date with upstream/master

@aahna-ashina
Copy link
Member

aahna-ashina commented Nov 19, 2023

@johnmark13 It says 91 files changed?

Will all those changes also appear once we make an upstream PR for the parent repo? Or only the files related to the Nation3 strategy change?

(Apologies if I'm missing some basic Git knowledge here.)

Copy link
Member

@aahna-ashina aahna-ashina left a comment

Choose a reason for hiding this comment

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

@johnmark13 I ran yarn test --strategy=nation3-passport-coop-with-delegations, and it says that two tests failed:

Test strategy "nation3-passport-coop-with-delegations" with example index 0 (latest snapshot) › Should return an array of object with addresses

    expect(received).toBeTruthy()

    Received: null

      168 |
      169 |     it('Should return an array of object with addresses', () => {
    > 170 |       expect(scores).toBeTruthy();
          |                      ^
      171 |       // Check array
      172 |       expect(Array.isArray(scores)).toBe(true);
      173 |       // Check array contains a object

      at Object.<anonymous> (test/strategy.test.ts:170:22)

Test Suites: 1 failed, 1 total
Tests:       2 failed, 2 skipped, 15 passed, 19 total
Snapshots:   0 total
Time:        54.209 s
Ran all test suites matching /strategy.test.ts/i.

Is it running successfully on your computer?

In case it matters, I'm using Yarn v4.0.0-rc.52 and Node v18.18.0

Copy link
Member

@aahna-ashina aahna-ashina left a comment

Choose a reason for hiding this comment

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

If more passports (besides ID 0) get revoked in the future, will this code continue to work?

@johnmark13 johnmark13 merged commit 2e3f996 into master Nov 25, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants