-
Notifications
You must be signed in to change notification settings - Fork 41
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
Pass Tenderly headers correctly #1241
Conversation
Pull Request Test Coverage Report for Build 8157719394Details
💛 - Coveralls |
} catch (error) { | ||
console.log(error); |
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.
console.log
left here.
@@ -70,7 +74,7 @@ export class TenderlyApi implements IAlertsApi { | |||
async deleteContract(contract: AlertsDeletion): Promise<void> { | |||
try { | |||
const url = `${this.baseUrl}/api/v1/account/${this.account}/project/${this.project}/contract/${contract.chainId}/${contract.address}`; | |||
await this.networkService.delete(url, { | |||
await this.networkService.delete(url, undefined, { |
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.
This is out of the scope of this PR, but I'm considering it would be beneficial to accept arguments via an interface in this class functions (args: {...}
) as we do in other places, to avoid hardcoding undefined
which could be a bit error-prone.
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 agree that it's a good addition. I will keep in mind for a follow up!
d20dbd2
to
ce88e34
Compare
async delete<T>( | ||
baseUrl: string, | ||
data?: object, | ||
{ params, headers }: NetworkRequest = {}, | ||
): Promise<NetworkResponse<T>> { |
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.
Is this part of this PR 🤔 I would scope it only with the Tenderly changes.
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.
This was a rebase issue. It should now be resolved.
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.
👏
Summary
This fixes the Tenderly requests to pass the
body
andheaders
correctly.Changes
addContract
/deleteContract
network request payloads