search

yearn / yearn-sdk

🦧 SDK for the yearn.finance platform. WIP
https://npm.im/@yfi/sdk
MIT License
53 stars 56 forks source link

fix: make partnerId optional in context #256

Closed jstashh closed 2 years ago

jstashh commented 2 years ago

Previously the SDK immediately crashed when instantiated without a partnerId being provided in the Context object. partnerId should be something not required to use the SDK.

We need to give it a value because export class Context implements Required<ContextValue> { uses Required, so it can't be optional. Although maybe we should remove this limitation

github-actions[bot] commented 2 years ago

size-limit report 📦

Path Size
dist/sdk.cjs.production.min.js 43.17 KB (+0.02% 🔺)
dist/sdk.esm.js 43.38 KB (-0.01% 🔽)
xgambitox commented 2 years ago

Removing the limitation might be best imo. If this fix its easier for now, lets go with it, but lets make sure it does not affect in some way in other places. Seems like it might be an issue here https://github.com/yearn/yearn-sdk/blob/master/src/services/zapper.ts#L265 ?

jstashh commented 2 years ago

@xgambitox won't that evaluate to false since it's an empty string?

xgambitox commented 2 years ago

@xgambitox won't that evaluate to false since it's an empty string?

Yes you are right. Right now seems to be no problem then, but in the future there is the possibility an error is introduced by a dev using partnerId === undefined.