Push to PRD #232
Merged
Push to PRD #232
Conversation
This file contains hidden or 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
Collaborator
bitbeckers
commented
Jan 30, 2025
bitbeckers
commented
- Expose collections entities
- Add currency amount to Sales entity
- geoJSON support and SDK bump
- Fix allowlist check in MetadataUploader (409 bug)
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.
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 #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.
Updates hypercerts SDK with latest version that supports geoJSON properties in metadata. Upload the accepted MIME types in the upload middleware to support the uploading of geoJSON files.
latest beta with formatter fix
geoJSON support
…phql Create `collections` top level entity in GraphQL
…y-amount feat: expose sales currency amount
reverts a regression introduced in commit 4f6beba where we started checking for allowlist, instead of metadata.allowlist. We should actually check for the presence of metadata.allowlist, to prevent the case where the user specifies both a CID and a json allowlist to the endpoint.
…list fix: check for presence of metadata.allowlist instead of allowlist
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.