Skip to content
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

fix/simplify funding loop #6749

Merged
merged 4 commits into from Jan 8, 2024
Merged

Conversation

MementoRC
Copy link
Sponsor Contributor

Before submitting this PR, please make sure:

  • Your code builds clean without any errors or warnings
  • You are using approved title ("feat/", "fix/", "docs/", "refactor/")

A description of the changes proposed in the pull request:
This is to try to understand what is wrong with this PR from fede

Tests performed by the developer:

Tips for QA testing:

@cardosofede
Copy link
Contributor

@MementoRC thanks for this work memento!
I didn't test asyncio.condition, it's like a flag? We can keep using this mechanism but we can remove the success flag since we can collect it later and if not we are going to block the bot with this process.
Btw, @carlitogetaladajr @fengtality I asked Memento to do the investigation around this issue and I think that might be considered a P2 bounty.

Copy link
Contributor

@rapcmia rapcmia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

  • Funding payment for the following connectors has been monitored and observed ok
    • Binance perpetual
    • Gateio perpetual
    • Kucoin perpetual
    • dYdX perpetual

@MementoRC
Copy link
Sponsor Contributor Author

@cardosofede The Condition adds a lock to the Event. It could be used to prevent the apparent race condition that occurred.
The other PR on using a timed-looping is also updated and seem to work, but an odd error is preventing the checks to pass.
It is not clear which method is more robust to me, so I simply updated both

@cardosofede
Copy link
Contributor

@MementoRC I think that we should go with this PR. Please clean the test that are commented and we are ready to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants