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.

Sign up for GitHub

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

Jump to bottom

OCT-1895: basic holonym integration #393

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

OCT-1895: basic holonym integration #393

wants to merge 10 commits into from

Conversation

paulperegud
Copy link
Contributor

Description

Definition of Done

  1. Acceptance criteria are met.
  2. PR is manually tested before the merge by developer(s).
    • Happy path is manually checked.
  3. PR is manually tested by QA when their assistance is required (1).
    • Octant Areas & Test Cases are checked for impact and updated if required (2).
  4. Unit tests are added unless there is a reason to omit them.
  5. Automated tests are added when required.
  6. The code is merged.
  7. Tech documentation is added / updated, reviewed and approved (including mandatory approval by a code owner, should such exist for changed files).
    • BE: Swagger documentation is updated.
  8. When required by QA:
    • Deployed to the relevant environment.
    • Passed system tests.

(1) Developer(s) in coordination with QA decide whether it's required. For small tickets introducing small changes QA assistance is most probably not required.

(2) Octant Areas & Test Cases.

@paulperegud paulperegud force-pushed the add-holonym-integration branch from b92e3fd to 3815cda Compare August 26, 2024 09:36
@paulperegud paulperegud changed the title [DRAFT] Add holonym integration OCT-1895: basic holonym integration Aug 26, 2024
@paulperegud paulperegud force-pushed the add-holonym-integration branch 3 times, most recently from 2a2d4c3 to 7001869 Compare August 26, 2024 10:35
kgarbacinski
kgarbacinski reviewed Aug 27, 2024
View reviewed changes
backend/app/infrastructure/database/models.py
id = Column(db.Integer, primary_key=True)
user_id = Column(db.Integer, db.ForeignKey("users.id"), nullable=False)
has_sbt = Column(db.Boolean, nullable=False, default=False)
sbt_details = Column(db.String, nullable=False, default="[]")
Copy link
Contributor

Choose a reason for hiding this comment

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

should we set a default param as [] or just "" since it's a string and it'd make the future validations a bit easier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field is processed as json. Both [] and "" are not a valid JSON string.

kgarbacinski
kgarbacinski reviewed Aug 27, 2024
View reviewed changes
backend/app/modules/uq/service/preliminary.py
@@ -12,21 +12,30 @@


@runtime_checkable
class Antisybil(Protocol):
class Passport(Protocol):
Copy link
Contributor

@kgarbacinski kgarbacinski Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
class Passport(Protocol):
class GPAntisybil(Protocol):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately one of proposed names clashes with some other name. I'll leave both as they are.

kgarbacinski
kgarbacinski reviewed Aug 27, 2024
View reviewed changes
backend/app/modules/uq/service/preliminary.py
def get_antisybil_status(
self, _: Context, user_address: str
) -> Optional[Tuple[float, datetime]]:
...


@runtime_checkable
class Holonym(Protocol):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class Holonym(Protocol):
class HolonymAntisybil(Protocol):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately one of proposed names clashes with some other name. I'll leave both as they are.

kgarbacinski
kgarbacinski reviewed Aug 27, 2024
View reviewed changes
package.json
@@ -25,7 +25,7 @@
"apitest:logs": "docker compose -p apitest -f ./localenv/docker-compose.yaml -f ./localenv/apitest.yaml logs",
"apitest:run": "docker compose -p apitest -f ./localenv/docker-compose.yaml -f ./localenv/apitest.yaml run backend-apitest",
"apitest:clean": "docker rm -v -f $(docker ps -qa --filter 'name=apitest') || true",
"preapitest:up": "yarn apitest:clean",
"preapitest:up": "yarn apitest:clean; rm -f backend/dev.db",
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we don't need rm -f backend/dev.db in the general API testing config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since apitest is just spiced up unit test, it runs in test env and thus, it uses sqlite, that particular file. This went undetected for a long time, but if dev.db already has some data, test may crash. This could be a case on a dev machine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, actual solution is making sure that dev.db is not being copied into apitest-backend docker container. Will fix this.

kgarbacinski
kgarbacinski reviewed Aug 27, 2024
View reviewed changes
backend/app/infrastructure/external_api/holonym/antisybil.py
E_PASSPORT_ON_DEVICE_WITH_ZK = "epassport"


def check(
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not sure if it shouldn't be directly in the service since it's something more about the functional logic, not the API itself but just raising up the case, it's up to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All it does is iterate through credential type, enhancing API a bit. I wouldn't call that logic. Just presenting data in a bit more useful manner.

0xartem
0xartem reviewed Aug 27, 2024
View reviewed changes
Copy link
Member

@0xartem 0xartem left a comment

Choose a reason for hiding this comment

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

Looks good to me for the first pass, although there is some uncertainty now about including holonym in this release

kgarbacinski
kgarbacinski reviewed Aug 27, 2024
View reviewed changes
backend/app/infrastructure/database/models.py
@@ -52,6 +52,14 @@ class GPStamps(BaseModel):
stamps = Column(db.String, nullable=False)


class HolonymSBT(BaseModel):
Copy link
Contributor

Choose a reason for hiding this comment

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

i also started thinking if we shouldn't match HolonymSBT with a specific epoch that it corresponds to? it'll help to distinguish between these tokens that are valid and expired ones (since we want to verify it for every epoch specifically, right?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Holonym's API doesn't return information about expiration. We should refresh SBT status when user allocates.

@paulperegud paulperegud force-pushed the add-holonym-integration branch 2 times, most recently from 7872931 to c64bb47 Compare August 28, 2024 14:11
paulperegud and others added 5 commits August 28, 2024 17:16
@paulperegud
add holonym_sbts table
7c08a71
@paulperegud
refactor: we will have multiple antisybil services
31ecfff
@paulperegud
add holonym integration (fetch, cache, expose status via API)
01ca3df
@paulperegud @kgarbacinski
Update backend/app/infrastructure/external_api/holonym/antisybil.py
da3622b 
Co-authored-by: Kacper Garbaciński <[email protected]>
@paulperegud
cr: lots of minor changes
045a659 
* use DTO instead of tuples
* return partial results when fetching antisybil status - they are enough for FE
* lineup declared and actual types
@paulperegud paulperegud force-pushed the add-holonym-integration branch from 96829e4 to 77a91fe Compare August 28, 2024 15:16
@paulperegud
apitest: speedup env termination by enabling zombie process reaping
108d356 
* use `exec` when starting multideployer
* run services that struggle with reaping with dedicated PID 1 process (https://blog.phusion.nl/2015/01/20/docker-and-the-pid-1-zombie-reaping-problem/)

This two measures drops shutdown time from 10s to 4s.
@paulperegud
add service unit test
a33a0fc
@paulperegud paulperegud force-pushed the add-holonym-integration branch from 77a91fe to a33a0fc Compare August 28, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgarbacinski kgarbacinski kgarbacinski left review comments

@0xartem 0xartem 0xartem left review comments

@housekeeper-bot housekeeper-bot Awaiting requested review from housekeeper-bot housekeeper-bot is a code owner

@adam-gf adam-gf Awaiting requested review from adam-gf

At least 2 approving reviews are required to merge this pull request.

Assignees

@paulperegud paulperegud

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paulperegud @0xartem @kgarbacinski