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

Add pallet-signal #12

Merged
merged 11 commits into from
Mar 13, 2022
Merged

Add pallet-signal #12

merged 11 commits into from
Mar 13, 2022

Conversation

vayesy
Copy link
Contributor

@vayesy vayesy commented Feb 24, 2022

Pallet Signal migration.

DONE:

  • Pallet Signal code migration.
  • Mocks, tests.

TODO:

  • Benchmarks (blocked by Control pallet)

Next steps:

  1. Merge Flow, Signal, Control feature branches.
  2. Test everything together, update zero-protocol runtime.
  3. Flow benchmarks, Signal benchmarks.
  4. Control tests and benchmarks.
  5. Hardening.

@vayesy vayesy changed the title Add pallet Signal WIP: Add pallet Signal Feb 24, 2022
@vayesy vayesy self-assigned this Feb 24, 2022
@vayesy vayesy force-pushed the migration/signal branch from e0d0b18 to 3149961 Compare March 2, 2022 17:46
@vayesy vayesy requested review from vovacha and 2075 March 11, 2022 20:54
@vayesy
Copy link
Contributor Author

vayesy commented Mar 11, 2022

@2075 there are few moments I would like to clarify before finalizing this piece.
It would be nice if you could address those moments.

  1. unlock_balance
    Currently this helper method is called in on_finalize as well as when calling simple vote extrinsic. As far as I understand from the pallet logic, this should be called only when proposal expires in on_finalize and should not be called in extrinsic. Correct me if I am wrong.
  2. Text errors
    There are few checks in extrinsic, which generate text errors instead of specific error enum value, example 1, example 2. Were these errors generated like this on purpose or it is fine to replace them with one of Error enum values?
  3. Equal amount of votes
    It is possible to have equal number of yes/no votes, example. What should happen with proposal in that case, should it be considered as accepted or declines?

@vovacha vovacha marked this pull request as ready for review March 13, 2022 14:31
@vovacha vovacha changed the title WIP: Add pallet Signal Add pallet-signal Mar 13, 2022
@vovacha vovacha self-assigned this Mar 13, 2022
Copy link
Contributor

@vovacha vovacha left a comment

Choose a reason for hiding this comment

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

LGTM. We still have a bunch of open questions which are not critical for now. I'd like to merge it, since the build is stable and tested. But during the hardening phase we should finalize our discussion and make appropriate changes to the code base.

@@ -0,0 +1,23 @@
use frame_support::pallet_prelude::{Encode, Decode};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should keep structs and enums in separate places. For now, it's not critical, but let's discuss it later and use the same approach for all pallets.

pub const ACC2: AccountId = 2;
pub const TREASURY_ACC: AccountId = 3;

pub struct ControlFixture {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a reminder: remove these fixtures after Control and Flow will be merged.

@vovacha vovacha merged commit 818b321 into main Mar 13, 2022
@vovacha vovacha linked an issue Mar 13, 2022 that may be closed by this pull request
12 tasks
@2075
Copy link
Member

2075 commented Mar 14, 2022

1. **`unlock_balance`**
   Currently this [helper method](https://github.com/playzero/subzero/blob/dev/modules/signal/src/lib.rs#L567) is called in `on_finalize` as well as when calling simple vote extrinsic. As far as I understand from the pallet logic, this should be called only when proposal expires in `on_finalize` and should not be called in extrinsic. Correct me if I am wrong.

The funds are collected from the contributors first, therefore they are unreserved and transferred to destination wallet.
after collecting the funds are reserved again and only a fraction unreserved to cover the settlement fees.

2. **Text errors**
   There are few checks in extrinsic, which generate text errors instead of specific error enum value, [example 1](https://github.com/playzero/subzero/blob/dev/modules/signal/src/lib.rs#L255), [example 2](https://github.com/playzero/subzero/blob/dev/modules/signal/src/lib.rs#L243). Were these errors generated like this on purpose or it is fine to replace them with one of Error enum values?

undecided about these, saw it in other places as well. in general nothing should panic in a live system, therefore i consider those which are more like an internal sanity check and nothing that throws / generates an event, can be text. what do you think?

3. **Equal amount of votes**
   It is possible to have equal number of yes/no votes, [example](https://github.com/playzero/subzero/blob/dev/modules/signal/src/lib.rs#L670). What should happen with proposal in that case, should it be considered as accepted or declines?

based on the current implementation it is a draw and could expire. in that case eventually a restart should be possible?

match &proposal.proposal_type {

we could/should also prepare already for different preset ratios and create enums for there, like more than 1/4, 2/4, 3/4, 4/4 etc.

@vayesy vayesy deleted the migration/signal branch March 18, 2022 16:09
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.

beta/dev: migrate signal pallet to 3.0
3 participants