search

joticajulian / kondor-website

MIT License
1 stars 0 forks source link

SC: Review Frank's Smart Contract Repository and give feedback #9

Open pgarciagon opened 7 months ago

joticajulian commented 7 months ago

Congrats for the progress in the contracts! there is a lot of work already done and the result is very good! Here I put some points to fix:

Do not restrict deallocation

https://github.com/Volano-DAO/Volano-Contracts/blob/04c02b1ae1703f0b37c6c8f1b01dade44b13326b/packages/treasury/assembly/treasury-pool.ts#L191-L192

The user should be able to deallocate at any time, even if the proposal has not started (upcoming proposal) or if the proposal has already ended (expired proposal).

Calculation of shares

https://github.com/Volano-DAO/Volano-Contracts/blob/04c02b1ae1703f0b37c6c8f1b01dade44b13326b/packages/treasury/assembly/treasury-pool.ts#L322-L326

Divide by allocatedGovernanceTokens, do not add the amount yet. For the maths check this comment from Fogata.

Process proposal

https://github.com/Volano-DAO/Volano-Contracts/blob/04c02b1ae1703f0b37c6c8f1b01dade44b13326b/packages/treasury/assembly/treasury-pool.ts#L461-L464

Change this to: Require last_paid_at + 1 month < DateTime.now()

Also, remember to initialize last_paid_at during the creation of the proposal: last paid at = start at - 1 month

https://github.com/Volano-DAO/Volano-Contracts/blob/04c02b1ae1703f0b37c6c8f1b01dade44b13326b/packages/treasury/assembly/treasury-pool.ts#L493-L496

This require should be the opposite: Require allocation > proposal.amount

https://github.com/Volano-DAO/Volano-Contracts/blob/04c02b1ae1703f0b37c6c8f1b01dade44b13326b/packages/treasury/assembly/treasury-pool.ts#L526-L530

Here you should burn the amount of vapor, that is, allocatedGovernanceTokens!.amount

Vault per proposal

This is just an idea, I have to think more about it. I see that right now you have 1 vault per pool. However, this is not useful to compute the VAPOR allocated on each proposal and for that you have to use the stateManager. Wouldn't be easier to setup a vault for each proposal? at the end what we want is to compute how much vapor is on each proposal, and people should be able to withdraw vapor or deposit vapor into the proposal.

Users per proposal

Do you have a function to get the list of users given a specific proposal? we need this function for one of the pages of the website. My suggestion is to save the user list in a space of each proposal. In this way it's easier to consult them.

Guardians

I'm not sure if the guardians are useful for this Vapor System. Here the door is open to everyone without restrictions. Let me know your thoughts.

frank-weijers commented 7 months ago

Hi Julian, thanks for your feedback. Happy to hear you like the results so far!

I will implement your feedback for these items:

For the burning amount: this should only be the used amount of the allocatedGovernanceTokens right? Because allocated vapor should usually be higher then the required amount. So just using allocatedGovernanceTokens!.amount would then be incorrect?

Vault per proposal I didn't consider this but I think it's actually a very good idea! This way the deposit/withdrawal logic can fully go inside the Vault where it's more suitable. I will have to implement the shares logic inside the vault then but I think it will also benefit other protocols that will be using the vault.

I will also have to do some refactoring because the vault are now all hard-coded into the contracts unlike the states of the state manager. The vaults will have to be configurable in the same way states are using configs.

It's a bit of extra work so I will finish the contract first with the current setup.

Users per proposal

The account state manager is created for this purpose. To have a way to easily update and retrieve user states. I have implemented multiple ways of doing so to provide maximum flexiblity. If there is still something missing the account state manager should be able to provide it rather than creating a new state for it.

listAccounts - List accounts with their resources and nested states: https://github.com/Volano-DAO/Volano-Contracts/blob/04c02b1ae1703f0b37c6c8f1b01dade44b13326b/packages/state-manager/assembly/__tests__/account-state-manager-list.spec.ts#L33

listStates - List states for an account, returns full states: https://github.com/Volano-DAO/Volano-Contracts/blob/04c02b1ae1703f0b37c6c8f1b01dade44b13326b/packages/state-manager/assembly/__tests__/account-state-manager-list.spec.ts#L102

listAccountAmounts - List only amounts from states for an account: https://github.com/Volano-DAO/Volano-Contracts/blob/04c02b1ae1703f0b37c6c8f1b01dade44b13326b/packages/state-manager/assembly/__tests__/account-state-manager-list.spec.ts#L164

message amount {
  uint64 amount = 1 [jstype = JS_STRING];
  oneof contract_oneof {
    contract contract = 2;
  }
}

listAccountStateResults - List all states with their keys + amounts for an account: https://github.com/Volano-DAO/Volano-Contracts/blob/04c02b1ae1703f0b37c6c8f1b01dade44b13326b/packages/state-manager/assembly/__tests__/account-state-manager-list.spec.ts#L194

message result {
  string resourceName = 1;
  string key = 2;
  string unit = 3;
  uint64 amount = 4 [jstype = JS_STRING];
  oneof contract_oneof {
    contract contract = 5;
  }
}

listAmounts - list amounts for all accounts for a resource + state key combination: https://github.com/Volano-DAO/Volano-Contracts/blob/04c02b1ae1703f0b37c6c8f1b01dade44b13326b/packages/state-manager/assembly/__tests__/account-state-manager-list.spec.ts#L306

message amount_with_account {
  bytes address = 1 [(koinos.btype) = ADDRESS];
  uint64 amount = 2 [jstype = JS_STRING];
  oneof contract_oneof {
    contract contract = 3;
  }
}

listStateResults - list state results for multiple accounts for a resource + state key combination: https://github.com/Volano-DAO/Volano-Contracts/blob/04c02b1ae1703f0b37c6c8f1b01dade44b13326b/packages/state-manager/assembly/__tests__/account-state-manager-list.spec.ts#L392

message result_with_account {
  bytes address = 1 [(koinos.btype) = ADDRESS];
  string resourceName = 2;
  string key = 3;
  string unit = 4;
  uint64 amount = 5 [jstype = JS_STRING];
  oneof contract_oneof {
    contract contract = 6;
  }
}

Guardian

The guardian now only uses the timer guard to lock actions for expired proposals. These guard settings should be removed as you mentioned. However, I would still like to include the guardian inside the module to provide more flexibility for the treasury module.

By including it other implementing contracts can benefit from it. They could leverage it to add guards that could be developed in the future or even add their own guards.

They could for example restrict the actions contribute, allocate, deallocate and create proposals to certain users. Like NFT holders, or only submit x proposals within a period.