-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add fees retrieval #114
Add fees retrieval #114
Conversation
if (!value) { | ||
return null; | ||
} | ||
return { streamflowFee: Number(value) / 100, partnerFee: 0 }; |
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.
Should we divide here with BN or are we ok with this approach?
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.
BN does not support decimal points.
@@ -262,4 +264,18 @@ export default class GenericStreamClient<T extends IChain> extends BaseStreamCli | |||
this.nativeStreamClient.extractErrorCode | |||
); | |||
} | |||
|
|||
/** | |||
* Returns streamflow and partner fees for the specific wallet in % |
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.
Nit, since it's public SDK, we should capitalize S in Streamflow
/** | ||
* Returns default Streamflow Fee in % | ||
*/ | ||
public getDefaultStreamflowFee(): Promise<number> { |
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'd move all of this out from StreamClient into some Fees file
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.
It's all part of the protocol, like for EVM we interact directly with our program for example. It would be probably better rename streamClient to Protocol Client though.
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.
But it sounds like a part of bigger rework and I am not sure when we'll get to it (and who will own in).
|
||
public async getDefaultStreamflowFee(): Promise<number> { | ||
const fee = await this.readContract.getStreamflowFees(); | ||
return fee.toNumber() / 100; |
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.
Even better to divide before moving to number :)
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 would need to add a dependency then, because EVM returns BigNumber
return null; | ||
} | ||
return { | ||
streamflowFee: Number(filteredPartners[0].strm_fee.toFixed(4)), |
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.
Why 4?
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.
To be safe, though up to 2 should be enough, because that's how much we support on on aptos/sui/evm
No description provided.