NCN Config Setup #1
Conversation
core/src/ncn_config.rs
Outdated
| } | ||
|
|
||
| pub fn seeds() -> Vec<Vec<u8>> { | ||
| vec![b"config".to_vec()] |
There was a problem hiding this comment.
My suggestion, PDA this off of the NCN as well
There was a problem hiding this comment.
Good idea if we ever need to switch NCNs
There was a problem hiding this comment.
still needs to be addressed
There was a problem hiding this comment.
fixed seeds, was already changed in other places
core/src/ncn_config.rs
Outdated
| /// This is important so all operators calculate the same Merkle root regardless of when fee changes take place. | ||
| #[derive(Debug, Clone, Copy, Zeroable, ShankType, Pod)] | ||
| #[repr(C)] | ||
| pub struct Fees { |
There was a problem hiding this comment.
nit: split out Fee/Fees into a separate file
core/src/ncn_config.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn set_new_fees( |
There was a problem hiding this comment.
It seems that we can update the fees multiple times per epoch. Meaning we won't have consistent fees across epochs.
Also, just a thought on design, thoughts on making the Fee/Fees generic and then we have one Fees for each of our fees - so they can be updated independently?
There was a problem hiding this comment.
Ah good catch, just fixed the logic so current fees are copied over first and then new fees are applied.
I prefer having it as a single Fee, saves the need for additional epochs to be stored. But small difference either way
There was a problem hiding this comment.
actually maybe I misunderstood, what's wrong with multiple fee changes per epoch?
| load_system_program(system_program)?; | ||
|
|
||
| Ncn::load(restaking_program_id.key, ncn_account, false)?; | ||
|
|
There was a problem hiding this comment.
Check the ncn.ncn_admin is the signer
There was a problem hiding this comment.
Was thinking, is this actually important? NCN admin will have the privileges over the config whether it creates the config or not, so no need to have the extra constraint.
Though in a world where multiple configs can exist maybe we want some restriction?
There was a problem hiding this comment.
I think it makes sense for all of the config actions to be permissioned, but that's just my nit
|
Still to add: set_new_admin ix, and unit tests |
coachchucksol
left a comment
coachchucksol
left a comment
There was a problem hiding this comment.
2 changes:
- ncn_admin should be signer to initialize the NCN config since we can set fees there
- PDA the ncn config off of the NCN
core/src/instruction.rs
Outdated
| /// Initialize the global configuration for this NCN | ||
| #[account(0, writable, name = "config")] | ||
| #[account(1, name = "ncn")] | ||
| #[account(2, writable, name = "ncn_admin")] |
There was a problem hiding this comment.
should make this signer - we're setting fees here
There was a problem hiding this comment.
was already happening in the instruction, now reflected in this file
core/src/ncn_config.rs
Outdated
| } | ||
|
|
||
| pub fn seeds() -> Vec<Vec<u8>> { | ||
| vec![b"config".to_vec()] |
There was a problem hiding this comment.
still needs to be addressed
|
|
||
| const fn current_fee(&self, current_epoch: u64) -> &Fee { | ||
| // If either fee is not yet active, return the other one | ||
| if self.fee_1.activation_epoch > current_epoch { |
There was a problem hiding this comment.
Is there a way to have both fees 'non-active'?
Uh oh!
There was an error while loading. Please reload this page.