Skip to content

Conversation

@aschmahmann
Copy link
Contributor

Using a blockstore isn't really the right interface here (e.g. Has doesn't really make any sense). The exchange.Interface or better the exchange.Fetcher interfaces are more appropriate and less confusing.

This also removes all the notImplemented errors. The functions not implemented are essentially optional, so descriptions as to what they are for have been left there but they otherwise should not cause issues.

Note: there seems to be some good cause here to simplify the examples (and make them more production-like) by adding:

  • An implementation of the exchange interface that takes an exchange.Fetcher.
  • An implementation of an LRU blockstore

Both of these are effectively implemented in bifrost-gateway (ipfs-inactive/bifrost-gateway#61). So once we're happy with how things pan out there we should consider porting implementations back to boxo.

Explicitly looking for feedback such as:

  • Things to fix
  • Comments to add/remove
  • If we should hold off on merging this until some of the bifrost-gateway pieces have moved back here so the example is more production-like

Other feedback welcome too 😄

@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #229 (928df5d) into main (2835752) will increase coverage by 0.12%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #229      +/-   ##
==========================================
+ Coverage   48.02%   48.14%   +0.12%     
==========================================
  Files         269      269              
  Lines       32957    32962       +5     
==========================================
+ Hits        15827    15870      +43     
+ Misses      15454    15428      -26     
+ Partials     1676     1664      -12     
Impacted Files Coverage Δ
examples/gateway/proxy/main.go 0.00% <0.00%> (ø)
examples/gateway/proxy/blockstore.go 49.31% <40.00%> (ø)

... and 9 files with indirect coverage changes

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

LGTM. Tbf, when I wrote this for the first time I wasn't really aware of the other abstractions (like the exchange). It does indeed make more sense to use it.

Base automatically changed from feat/gateway-refactor to main March 29, 2023 18:47
@hacdias hacdias requested a review from a team as a code owner March 29, 2023 18:47
@hacdias hacdias force-pushed the feat/cleanup-gateway-example branch 3 times, most recently from e56fa3b to aa107c6 Compare March 31, 2023 09:20
@hacdias hacdias changed the base branch from main to ci/gateway-conformance-subdomain March 31, 2023 09:20
@hacdias
Copy link
Member

hacdias commented Mar 31, 2023

I rebased on top of #246 since I made quite a few changes to the examples there and we enabled more conformance tests, just to run more tests here.

@hacdias hacdias changed the title switch gateway CAR example to use a proxy exchange instead of blockstore feat(examples): switch CAR gateway to use a proxy exchange instead of blockstore Mar 31, 2023
@hacdias hacdias changed the title feat(examples): switch CAR gateway to use a proxy exchange instead of blockstore feat(examples): switch proxy gateway to use exchange instead of blockstore Mar 31, 2023
@hacdias hacdias force-pushed the ci/gateway-conformance-subdomain branch from 0862a5f to 725a8f4 Compare March 31, 2023 09:38
@hacdias hacdias force-pushed the feat/cleanup-gateway-example branch from aa107c6 to 028e9a5 Compare March 31, 2023 09:39
Base automatically changed from ci/gateway-conformance-subdomain to main March 31, 2023 12:24
@lidel lidel force-pushed the feat/cleanup-gateway-example branch from 028e9a5 to 928df5d Compare March 31, 2023 12:35
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Rebased since bunch of refactor had overlap with #246, now this PR is much smaller. lgtm

@hacdias hacdias changed the title feat(examples): switch proxy gateway to use exchange instead of blockstore refactor(examples): proxy gateway to use exchange Mar 31, 2023
@hacdias hacdias merged commit b1046dd into main Mar 31, 2023
@hacdias hacdias deleted the feat/cleanup-gateway-example branch March 31, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants