Create collections top level entity in GraphQL
#214
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pheuberger
force-pushed
the
create-collections-entity-graphql
branch
from
079b027 to
268f190
Compare
pheuberger
force-pushed
the
create-collections-entity-graphql
branch
from
268f190 to
fd6d6bb
Compare
pheuberger
force-pushed
the
create-collections-entity-graphql
branch
from
fd6d6bb to
9e80918
Compare
Jipperism
reviewed
Jan 21, 2025
This sort function was only built to work with a few very specific types from the Caching DB. However, it was used on data that wasn't meant for the caching DB. Namely the Blueprints and perhaps others. This implements basic sorting for all database types.
pheuberger
force-pushed
the
create-collections-entity-graphql
branch
from
03fb317 to
e41e9cb
Compare
|
@bitbeckers Unit tests are failing now given the coverage requirements, which is blocking this PR. What's the best way forward? |
|
@pheuberger where does the regression come from? If it's about the Services we can lower the threshold as we'll soon focus on #218
|
Yes, it must be the services, as that's the majority of code added outside of I ignored |
Our services are difficult to test at the moment and we don't really have tests for them. However, when you make changes to the GraphQL schema and potentially introduce new entities, you'll also alway have to add code to the services and might go below the thresholds set. PR hypercerts-org#218 is going to address this problem and make the services as well as some of the GraphQL testable. This is in anticipation of the next commit which is going to introduce the collections entity as a top-level entity.
Without this patch the user of our GraphQL endpoints needs to go via hyperboards to access the collections inside. Another drawback is that via this query they can only access the hypercert id, so they'll have to fire a separate query to combine the data. This patch adds collections as a top level entity and exposes full hypercert objects as child entities. This also applies to hyperboards as the standalone collections entity is being reused there too.
The problem right now is that the constants that are loaded from the environment have an additional name if specified. This additional name is very confusing if you're presented with the error message Environment variable Alchemy API key is not set. This makes you wonder if perhaps you have mistyped the variable name. It's almost always better to just print the variable name as it is expected in your .env file. One notable exception is perhaps our Web3Up proof and secret. That's why this patch changes the approach of assertExists() to only deal with environment variables (it's not used in any other way currently) and have the caller specify the variable name and an optional hint to help the person stumbling over a missing env var error with additional context. The name of assertExists() was renamed to getRequiredEnvVar() to better reflect what the function is doing. The filename was also renamed to further support this.
The current test file was testing library code that is already comprehensively tested. It was also making slow RPC requests and slowed the test suite down substantially. It requires the Alchemy API key to be set which is not something that we want, so this patch is doing away with all superfluous tests and focuses on testing only the code that we've added around the library call. Also lowered the coverage thresholds since the EVM client getter is now mocked and its internals are thus not covered by these tests any longer.
Without this patch the Safe signature verification code will get RPC URLs for the specified chain id. This in turn will load the Alchemy API key, which is not relevant for our tests. This patch mocks all of this away, so that the API key doesn't need to be present in the environment while running tests. Also lowered the coverage threshold of functiosn to 52 as mocking getRpcUrl() doesn't touch the internals of this function. This should be thoroughly tested in a dedicated test file anyway.
pheuberger
force-pushed
the
create-collections-entity-graphql
branch
from
e41e9cb to
cfeb432
Compare
This was referenced Jan 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Without this PR the user of our GraphQL endpoints needs to go via hyperboards to access the collections inside. Another drawback is that via this query they can only access the hypercert id, so they'll have to fire a separate query to combine the data.
This PR adds collections as a top level entity and exposes full hypercert objects as child entities. This also applies to hyperboards as the standalone collections entity is being reused there too.
This PR also provides a fix for hyperboard sorting.
Incidentally the tests around signature verification were broken, which blocked this PR. I refactored the tests to not rely on the Alchemy API key to fix the issues.