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

Implement CommissionModel #1584

Merged
merged 1 commit into from
Apr 9, 2024
Merged

Conversation

rsmb7z
Copy link
Collaborator

@rsmb7z rsmb7z commented Apr 9, 2024

Pull Request

This PR introduces an enhancement to the CommissionModel, as outlined in issue #632, maintaining the default behavior while integrating a fixed price model. It further establishes a foundation for developing adapter-specific commission models.

Type of change

  • New feature (non-breaking change which adds functionality)

How has this change been tested?

Existing tests passed and new tests added.

@cjdsellers
Copy link
Member

Very nicely done, I think we can merge this as is - I appreciate the tests 🙏

@cjdsellers cjdsellers merged commit 5258143 into nautechsystems:develop Apr 9, 2024
11 checks passed
@rsmb7z rsmb7z deleted the pr_240409 branch April 9, 2024 13:51
@cjdsellers
Copy link
Member

Just following up on this.

I've completed a rename of CommissionModel to FeeModel, just because commissions are only one component of a fee model - I left the get_commission method name unchanged though.

I also think you named InstrumentSpecificPercentCommissionModel well, however the direction we're taking the platform will be to take the fees off the instrument definitions - as they are more applicable at the venue listing level.

So I renamed that to MakerTakerFeeModel.

We could expand the FeeModel to include funding rates/fees (or that could be a separate model), but you did a great job of wiring this through, making the next step of removing fees from instruments much easier 🙏.

@rsmb7z
Copy link
Collaborator Author

rsmb7z commented Apr 10, 2024

I've completed a rename of CommissionModel to FeeModel, just because commissions are only one component of a fee model - I left the get_commission method name unchanged though.

Thanks, thats perfectly aligns with broader fee concept. I think we may change the other one too to either FixedFeeModel or ConstantFeeModel.

I also think you named InstrumentSpecificPercentCommissionModel well, however the direction we're taking the platform will be to take the fees off the instrument definitions - as they are more applicable at the venue listing level.

So I renamed that to MakerTakerFeeModel.

Yes at that stage, maker/taker fee would become direct input of the class. Also because this is added when adding the Venue so advanced model can have a lot more complex logic i.e. dynamic rates based on monthly volume etc. I will be adding IB one soon which includes dynamic commission calculation based on Instrument.

@cjdsellers
Copy link
Member

I also took your suggestion of renaming FixedCommissionModel to FixedFeeModel, I think its cleaner.

Now on develop from c7c1a9e.

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

Successfully merging this pull request may close these issues.

2 participants