-
Notifications
You must be signed in to change notification settings - Fork 17
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: only return approved third parties #310
Conversation
Pull Request Test Coverage Report for Build 1343434039
💛 - Coveralls |
src/ethereum/api/fragments.ts
Outdated
maxItems | ||
totalItems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless we're creating another endpoint to get the max items and the total items, I would leave these properties here so we can use them in the UI to tell the user to buy more slots.
d280f73
to
3e9b73e
Compare
src/ThirdParty/utils.ts
Outdated
import { ThirdParty } from './ThirdParty.types' | ||
|
||
export function toThirdParty(fragment: ThirdPartyFragment): ThirdParty { | ||
const { name, description } = fragment.metadata.thirdParty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't use the object deconstruction here as thirdParty can be null.
src/ethereum/api/fragments.ts
Outdated
thirdParty: { | ||
name: string | ||
description: string | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property thirdParty can be null
src/ThirdParty/utils.spec.ts
Outdated
} | ||
}) | ||
|
||
it('should take a third party fragment and parse it to conform to the ThirdParty type', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the async here, as we're not using the await keyword:
it('should take a third party fragment and parse it to conform to the ThirdParty type', async () => { | |
it('should take a third party fragment and parse it to conform to the ThirdParty type', () => { |
@@ -74,7 +74,7 @@ async function decodeAuthChain(req: Request): Promise<string> { | |||
errorMessage = 'Missing ETH address in auth chain' | |||
} else { | |||
try { | |||
const endpoint = (req.method + ':' + req.url).toLowerCase() | |||
const endpoint = (req.method + ':' + req.path).toLowerCase() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any kind of test that verifies this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't actually but I felt like testing the entire authentication middleware was a bit overkill for this PR
Updated! |
This reverts commit 2089bf2.
It also removed __typename from requests as we're not using it and it's cluttering our server responses (from thegraph)