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: minor issues NTRN-402 #43

Merged
merged 13 commits into from
Mar 18, 2023

Conversation

sotnikov-s
Copy link
Contributor

@sotnikov-s sotnikov-s commented Mar 14, 2023

task: https://p2pvalidator.atlassian.net/browse/NTRN-402

subtask report:

  1. was done in feat: add lockdrop vault NTRN-343 #35;
  2. was done in feat: add lockdrop vault NTRN-343 #35;
  3. make owner mandatory for the mentioned contracts + for the lockdrop and credits vaults;
  4. done as suggested;
  5. done as suggested;
  6. don't get what auditors mean (checked_div doesn't return OverflowError but DivideByZeroError), so I've added a non-zero check and prettified errors handling;
  7. removed the unused structs;
  8. comment is removed since the feature is not going to be implemented (discussed with @oopcode);
  9. idk what kind of validation denom and min_period require and what's wrong with the vesting_denominator validation, so I kept the original validation but moved it to a Config struct method and added tests for it. If you have an idea please let me know.

relative PRs:

tests run: https://github.com/neutron-org/neutron-tests/actions/runs/4446787903

@sotnikov-s sotnikov-s changed the base branch from main to neutron_audit_informal_17_01_2023 March 14, 2023 15:55
@@ -95,32 +84,25 @@ pub fn execute_update_config(
deps: DepsMut,
info: MessageInfo,
new_credits_contract_address: Option<String>,
new_owner: Option<Admin>,
new_owner: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you change Admin to String?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

long story short: so it's simpler and common

simpler: because it's anyway eventually stored as Addr in the contract's cfg. just look a bit below at how this value was handled and how it's now handled. Admin struct is very complicated to be passed, just look at the changes here: https://github.com/neutron-org/neutron/pull/166/files. if we pass Admin::CoreModule, it simply sets the owner to the sender, so why don't allow sending the simple sender address?

common: another reason is that we have the owner field as String in all other contracts

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, i prefer to trust you!


#[error("Invalid vesting denominator")]
InvalidVestingDenominator(String),
#[error("vesting_denominator must be more than zero")]
Copy link
Contributor

Choose a reason for hiding this comment

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

more -> greater

@@ -21,10 +22,23 @@ pub struct Config {
/// Address of the security DAO contract
pub security_dao_address: Addr,

// Denomintator used int the vesting release function
// Denomintator used in the vesting release function
Copy link
Contributor

Choose a reason for hiding this comment

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

Denomintator -> Denominator

contracts/tokenomics/treasury/src/state.rs Show resolved Hide resolved
Copy link
Contributor

@NeverHappened NeverHappened left a comment

Choose a reason for hiding this comment

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

LGTM

@zavgorodnii zavgorodnii merged commit f585504 into neutron_audit_informal_17_01_2023 Mar 18, 2023
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.

3 participants