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

feat: retrieve impact reports #10

Merged
merged 4 commits into from
Jan 27, 2024
Merged

feat: retrieve impact reports #10

merged 4 commits into from
Jan 27, 2024

Conversation

baumstern
Copy link
Contributor

@baumstern baumstern commented Jan 20, 2024

Changes

  • Implemented a new feature to fetch existing impact reports from the Graph and IPFS via the Hypercert SDK. This is now available at the /impact-reports endpoint.

Future Work:

  • Properly define Report model
  • Implement server-side updates for newly added reports

Copy link
Collaborator

@thebeyondr thebeyondr left a comment

Choose a reason for hiding this comment

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

hey, please use functional programming tools over object-oriented ones. look into using functions, types and hooks instead of creating classes, models and controllers.

Copy link
Collaborator

@thebeyondr thebeyondr Jan 22, 2024

Choose a reason for hiding this comment

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

@baumstern waddup so I remade the connector in a functional way since I randomly woke up at midnight🤷🏽‍♂️. Feel free to edit as you please. I haven't removed any of your code so feel free to update that as needed. thanks for laying the foundation. This also isn't tested yet and needs to be before merging!

@CJ-Rose please assess this as well, and let me know if any questions/insights arise

The idea is that this acts as a resource route in Remix. Other UI routes can call /hypercert-reports and get data back

  • Re-add daehyuns hypercert documentation comments for gql
  • Setup UI to get data and cache it in session
  • IPFS speed check, it was slow even for hypercert docs (so is it viable to retrieve report data from it? hmmmm)
  • go back to sleep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for jumping in with the code! quick question: How's the caching handled with the functional approach? Used a singleton in my version to cut down on Graph/IPFS requests

Copy link
Collaborator

@thebeyondr thebeyondr Jan 22, 2024

Choose a reason for hiding this comment

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

caching isn't handled rn, thats the responsibility of the client. This is just responsible for returning that data.

Re: limiting requests, I'm sure there was still a loop fetching the metadata for each claim so it wasn't like a batched thing. How does a singleton help us batch requests? Also, can you point me to the singleton implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

caching isn't handled rn, thats the responsibility of the client. This is just responsible for returning that data.

Re: limiting requests, I'm sure there was still a loop fetching the metadata for each claim so it wasn't like a batched thing. How does a singleton help us batch requests? Also, can you point me to the singleton implementation?

server-side caching decouples the client from external services (such as Graph and IPFS), reducing redundant calls to these services. It retrieves and stores data upon instantiation.

It had implemented using static keyword:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I've scrapped the class implementation and replaced it with a module-scope variable, as it preserves data until the server process terminates. It's neater – thanks for the suggestion

Copy link
Collaborator

Choose a reason for hiding this comment

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

server-side caching decouples the client from external services (such as Graph and IPFS), reducing redundant calls to these services. It retrieves and stores data upon instantiation.

hmm, a few things:

  1. How do we allow the server cache to expire or tell it to fetch new data?
  2. I don't think this subverts the need for client-side caching. we need them both - what do you think?
  3. What type of test would allow us to verify that your implementation actually caches?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

server-side caching decouples the client from external services (such as Graph and IPFS), reducing redundant calls to these services. It retrieves and stores data upon instantiation.

hmm, a few things:

1. How do we allow the server cache to expire or tell it to fetch new data?

2. I don't think this subverts the need for client-side caching. we need them both - what do you think?

3. What type of test would allow us to verify that your implementation actually caches?
  1. the server should be notified when a Hypercert is minted, so it can update the entry
  2. sure, they serve different purposes
  3. I will add a test case

Copy link
Collaborator

Choose a reason for hiding this comment

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

awesome, thanks for your efforts so far. we're closer now 🫡

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't believe this was posted today, as we're talking about cache. 💥

https://youtu.be/RXMvnqb7A8k

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't believe this was posted today, as we're talking about cache. 💥

https://youtu.be/RXMvnqb7A8k

Is he Big Brother? 🤔🤔

@baumstern
Copy link
Contributor Author

hey, please use functional programming tools over object-oriented ones. look into using functions, types and hooks instead of creating classes, models and controllers.

main focus here was just nailing the functionality. But exploring functional programming now seems like a cool idea too -- I gotta say good bye to old school ways

@thebeyondr
Copy link
Collaborator

main focus here was just nailing the functionality. But exploring functional programming now seems like a cool idea too -- I gotta say good bye to old school ways

np, I just find functional programming ends up less buggy and verbose

@thebeyondr thebeyondr added the feature 🚀 New feature or request label Jan 22, 2024
@baumstern
Copy link
Contributor Author

@thebeyondr added tests, please take another look!

@baumstern baumstern changed the title feat: implement minimal server feat: retrieve impact reports Jan 26, 2024
Copy link
Collaborator

@thebeyondr thebeyondr left a comment

Choose a reason for hiding this comment

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

just a few small things then we're good!

package.json Outdated
@@ -7,13 +7,15 @@
"build": "remix vite:build",
"dev": "remix vite:dev",
"start": "remix-serve ./build/server/index.js",
"test": "vitest",
"lint": "biome check --apply ./app",
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 6373f32

@@ -30,6 +32,7 @@
"devDependencies": {
"@biomejs/biome": "1.5.1",
"@remix-run/dev": "^2.5.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh actually it's used in vite.config.ts

docs/server.md Outdated
@@ -0,0 +1,24 @@
# Server Design
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this here and not in README?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to README.md in 6373f32

Copy link
Collaborator

@thebeyondr thebeyondr left a comment

Choose a reason for hiding this comment

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

lfg

@thebeyondr thebeyondr merged commit 0f21312 into VoiceDeck:main Jan 27, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🚀 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants