-
Notifications
You must be signed in to change notification settings - Fork 10
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 #65: Improve balance lose check and mention it's details in docs #66
Conversation
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.
A small repeated typo
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.
Seems ok to me.
@@ -49,8 +49,8 @@ over the multi-asset order book to obtain a list of matches. The matches are the | |||
translated into transactions that will be signed and submitted by the bot. | |||
|
|||
Due to the open and decentralized design of the protocol, anybody can run a Smart Order | |||
Router instance and collect a share of the fees, thus running a Smart Order Router instance | |||
is not only contributiung to the further decentralization of the protocol, but it is also | |||
Router instance and collect the arbitrage opportunities, thus running a Smart Order Router instance |
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.
collect the arbitrage opportunities,
I think we should change the wording here. One does not "collect" opportunities.
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.
Sorry, could you elaborate / suggest the required 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.
Please see my comments.
Thanks for taking a look @adacapo21! The PR would be merged as soon as it is reviewed from someone else from team. |
You are welcome. Good luck! @sourabhxyz |
The proposed changes ensure that there is no Ada loss, even when taking fees into account, when the currency is lovelaces; and that when the trade involves a non-lovelace currency then the Ada loss is only due to fees paid. This seems reasonable to me. |
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.
Proposed changes ensure that when trade only involves lovelace as currency, there is no Ada loss even when taking fees into account.
Closes #65. Also see #62.