-
Notifications
You must be signed in to change notification settings - Fork 0
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
implements entry/redemption fees #55
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.
One question, should there be the events emitted when fees are changed?
pub struct PerformanceFeeUpdatedEvent { | ||
pub accountant_key: Pubkey, | ||
pub performance_fee: u64, | ||
} |
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 think it would be useful to also include timestamp to
PerformanceFeeUpdatedEvent, EntryFeeUpdatedEvent, RedemptionFeeUpdatedEvent
so that we can keep track of the fee changing history easily.
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.
don't we have a timestamp in the transaction data?
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.
true, No need to strictly use Clock sysvar timestamp.
@@ -53,22 +64,31 @@ impl Accountant for GenericAccountant { | |||
) | |||
} | |||
|
|||
fn set_fee(&mut self, fee: u64) -> Result<()> { | |||
fn set_performance_fee(&mut self, fee: u64) -> Result<()> { |
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 think for
set_performance_fee, set_redemption_fee, set_entry_fee fn
we can consider adding a condition that fee arg be less or equal to FEE_BPS constant value, just to ensure that the fee won't be over 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.
make sense, will add validation
|
||
#[account( | ||
mut, | ||
associated_token::mint = shares_mint, |
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.
The accountant requires a token account based on the share_mint. In my opinion, it would be nice if the InitTokenAccount context of the accountant program used a more generic naming convention for the mint account, rather than underlying_mint (which, in the context of this repository, refers to the underlying_asset that users can invest with). At the very least, the current naming caused some confusion for me.
No description provided.