-
Notifications
You must be signed in to change notification settings - Fork 11
chore: refine allbridge header #789
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
Conversation
kathaypacific
left a comment
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.
🪵
src/apps/allbridge/api.ts
Outdated
| (options) => { | ||
| logger.debug('allbridge: request details', { | ||
| url: options.url.toString(), | ||
| headers: options.headers, |
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 use any api tokens / authentication in the headers that could be logged here right?
src/apps/allbridge/api.ts
Outdated
| hooks: { | ||
| beforeRequest: [ | ||
| (options) => { | ||
| logger.debug('allbridge: request details', { | ||
| url: options.url.toString(), | ||
| headers: options.headers, | ||
| method: options.method, | ||
| }) | ||
| }, | ||
| ], | ||
| beforeError: [ | ||
| (error) => { | ||
| logger.error('allbridge: request error', { | ||
| statusCode: error.response?.statusCode, | ||
| statusMessage: error.response?.statusMessage, | ||
| responseHeaders: error.response?.headers, | ||
| responseBody: error.response?.body, | ||
| url: error.options?.url?.toString(), | ||
| requestHeaders: error.options?.headers, | ||
| }) | ||
| return error | ||
| }, | ||
| ], | ||
| afterResponse: [ | ||
| (response) => { | ||
| logger.debug('allbridge: response received', { | ||
| statusCode: response.statusCode, | ||
| headers: response.headers, | ||
| }) | ||
| return response | ||
| }, | ||
| ], | ||
| }, | ||
| }) |
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.
src/apps/allbridge/positions.ts
Outdated
| ? client | ||
| .readContract({ | ||
| address: tokenInfo.poolAddress, | ||
| abi: poolAbi, | ||
| functionName: 'balanceOf', | ||
| args: [address as Address], | ||
| }) | ||
| .catch((e) => { | ||
| logger.error('allbridge: failed to fetch balanceOf', e) | ||
| throw e | ||
| }) |
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 really need this?
jeanregisser
left a comment
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.
Thanks! 🚀
src/apps/allbridge/api.ts
Outdated
| .get('https://core.api.allbridgecoreapi.net/token-info') | ||
| .get('https://core.api.allbridgecoreapi.net/token-info', { | ||
| headers: { | ||
| 'x-Sdk-Agent': 'AllbridgeCoreSDK/0.0.0-development', // as in https://github.com/allbridge-io/allbridge-core-js-sdk/blob/5deb0623d6fffcbc7a85dba22b98942c47d4af6c/src/client/core-api/api-client.ts#L48 |
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.
Looks like the shipped libs have a real value: 3.21.0 for the latest version
https://www.npmjs.com/package/@allbridge/bridge-core-sdk?activeTab=code
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.
good catch, thanks!
Setting an allbridge request header similar to the official SDK in an attempt to get rid of the 403 response
Context: https://valora-app.slack.com/archives/C02N3AR2P2S/p1740482746542719