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

Feature/rebase democracy #965

Merged
merged 95 commits into from
Aug 23, 2023
Merged

Feature/rebase democracy #965

merged 95 commits into from
Aug 23, 2023

Conversation

PraetorP
Copy link
Contributor

No description provided.

@PraetorP PraetorP requested a review from mrshiposha July 10, 2023 09:04
runtime/common/config/pallets/governance.rs Outdated Show resolved Hide resolved
runtime/common/config/xcm/mod.rs Outdated Show resolved Hide resolved
runtime/quartz/Cargo.toml Show resolved Hide resolved
@@ -309,7 +302,7 @@ describe('Integration Test: Maintenance Functionality', () => {
},
]);
const preimage = helper.constructApiCall('api.tx.identity.forceInsertIdentities', [randomIdentities]).method.toHex();
preimageHashes.push(await notePreimage(helper, preimage));
preimageHashes.push(await helper.preimage.notePreimage(bob, preimage, true));
Copy link
Contributor

Choose a reason for hiding this comment

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

Хмм, по идее это не должно отличаться от текущего develop?

@@ -332,7 +325,7 @@ describe('Integration Test: Maintenance Functionality', () => {
const preimage = helper.constructApiCall('api.tx.balances.forceTransfer', [
{Id: zeroAccount.address}, {Id: superuser.address}, 1000n,
]).method.toHex();
const preimageHash = await notePreimage(helper, preimage);
const preimageHash = await helper.preimage.notePreimage(bob, preimage, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

И это, по идее, тоже не должно отличаться от текущего develop

@@ -104,6 +104,11 @@ export enum Pallets {
CollatorSelection = 'collatorselection',
Session = 'session',
Identity = 'identity',
Democracy = 'democracy',
Council = 'council',
//CouncilMembership = 'councilmembership',
Copy link
Contributor

Choose a reason for hiding this comment

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

Не помню почему это закомментировано

runtime/common/config/pallets/governance/types.rs Outdated Show resolved Hide resolved
runtime/common/config/pallets/governance/fellowship.rs Outdated Show resolved Hide resolved
runtime/common/config/pallets/governance/origins.rs Outdated Show resolved Hide resolved
runtime/common/config/pallets/governance/origins.rs Outdated Show resolved Hide resolved
runtime/common/config/pallets/governance/types.rs Outdated Show resolved Hide resolved
runtime/common/config/pallets/governance/types.rs Outdated Show resolved Hide resolved
runtime/common/config/pallets/governance/types.rs Outdated Show resolved Hide resolved
runtime/common/construct_runtime.rs Show resolved Hide resolved
pallets/configuration/src/lib.rs Outdated Show resolved Hide resolved
runtime/common/config/pallets/governance/types.rs Outdated Show resolved Hide resolved
runtime/common/config/pallets/governance/types.rs Outdated Show resolved Hide resolved
runtime/common/config/pallets/governance/types.rs Outdated Show resolved Hide resolved
runtime/common/construct_runtime.rs Show resolved Hide resolved
@PraetorP PraetorP marked this pull request as ready for review July 21, 2023 14:35
@mrshiposha mrshiposha added the CI-governance for CI workflows label Aug 11, 2023
@mrshiposha mrshiposha marked this pull request as draft August 11, 2023 13:55
@mrshiposha mrshiposha marked this pull request as ready for review August 11, 2023 13:55
tests/src/governance/council.test.ts Outdated Show resolved Hide resolved
});

itSub('Superuser can add a member', async ({helper}) => {
const [newMember] = await helper.arrange.createAccounts([0n], donor);
Copy link
Contributor

Choose a reason for hiding this comment

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

Я видел у нас добавился специальный метод для создания пустых акков. Наверное, его здесь использовать будет хорошо. В других похожих местах, думаю, тоже.

tests/src/governance/council.test.ts Outdated Show resolved Hide resolved

itSub('Superuser can add a member', async ({helper}) => {
const [newMember] = await helper.arrange.createAccounts([0n], donor);
await expect(helper.getSudo().executeExtrinsic(sudoer, 'api.tx.councilMembership.addMember', [newMember.address])).to.be.fulfilled;
Copy link
Contributor

Choose a reason for hiding this comment

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

У нас есть в плейграундах класс, который предоставляет методы для работы с membership. Там есть методы addMember/getMembers и прочие. Лучше использовать их, а не напрямую дергать executeExtrinsic. Во всех похожих местах.

itSub('[Negative] Council cannot submit regular democracy proposal', async ({helper}) => {
const councilProposal = await helper.democracy.proposeCall(dummyProposalCall(helper), 0n);

await expect(proposalFromCouncil(councilProposal)).to.be.rejectedWith('Proposal execution failed with BadOrigin');
Copy link
Contributor

Choose a reason for hiding this comment

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

Такие reject'ы у нас в других тестах обычно ловяться по regex'у:

Suggested change
await expect(proposalFromCouncil(councilProposal)).to.be.rejectedWith('Proposal execution failed with BadOrigin');
await expect(proposalFromCouncil(councilProposal)).to.be.rejectedWith(/BadOrigin/);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Не только. Можно просто строкой

Copy link
Contributor

Choose a reason for hiding this comment

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

Можно, но /BadOrigin/ покрывает вообще все случаи. В других тестах (не только governance) как правило используется именно такой вариант

Copy link
Member

@CertainLach CertainLach Aug 14, 2023

Choose a reason for hiding this comment

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

Строка тоже вхождение проверяет а не полное совпадение
Регулярки есть смысл использовать только для проверки полной ошибки: /^section\.BadOrigin$/

await expect(proposalFromCouncil(councilProposal)).to.be.rejectedWith('Proposal execution failed with BadOrigin');
});

itSub('[Negative] Cannot add a duplicate Council member', async ({helper}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Не уверен, что нам вообще нужен этот тест-кейс. Мы тут тестируем логику (которая написана не нами, а Парити), а не нашу настройку.

То же самое относится к другим тестам про мемберов-дубликатов

Copy link
Contributor

Choose a reason for hiding this comment

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

Да, поддерживаю, давайте уберем.
Итак слишком долго идут тесты


itSub('[Negative] Council member cannot add/remove a Council member', async ({helper}) => {
const [newCouncilMember] = await helper.arrange.createAccounts([0n], donor);
await expect(helper.council.membership.addMember(counselors.alex, newCouncilMember.address)).to.be.rejectedWith('BadOrigin');
Copy link
Contributor

Choose a reason for hiding this comment

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

Council Member (как и TechComm Member) -- это особенный origin соот-го коллектива, а не обычные signed origin'ы, соот-но, не смотря на то, что тест проходит, он тестирует не то, что нужно.

В данном случае, этот тест тестирует, что обычный signed-origin не может добавить/убрать советника. А нам нужное другое.

Здесь и в других "Council member" тестах нужно использовать экстринзик council.execute.

Для TechComm -- аналогично

Copy link
Contributor Author

Choose a reason for hiding this comment

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

То, что у них там как раз не совсем корректно (имхо) сделан адаптер Еншюр мамбер, я как раз заметил в тех ком. Тут не правил еще - поскольку решил это сделать после реализации тестов.

)).to.be.rejectedWith('BadOrigin');
await expect(helper.council.collective.execute(
counselors.alex,
helper.technicalCommittee.membership.removeMemberCall(counselors.charu.address),
Copy link
Contributor

Choose a reason for hiding this comment

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

Не того удаляем. Надо удалять члена тех-коммитета здесь. А в данном случае удаляется член совета из тех коммитета.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Вообще без разницы кого - мы смотрим, что это упадет с BadOrigin а не с какой либо другой ошибкой, тест не про то, что не можем удалить несуществующего мембера.

tests/src/governance/council.test.ts Outdated Show resolved Hide resolved
tests/src/governance/council.test.ts Outdated Show resolved Hide resolved
tests/src/governance/council.test.ts Outdated Show resolved Hide resolved
tests/src/governance/technicalCommittee.test.ts Outdated Show resolved Hide resolved
tests/src/governance/technicalCommittee.test.ts Outdated Show resolved Hide resolved
)).to.be.rejectedWith('Proposal execution failed with BadOrigin');
});

itSub('[Negative] TechComm member cannot blacklist Democracy proposals', async ({helper}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Надо бы еще добавить тест, что коммитет целиком НЕ может блеклистить. Я такого теста не вижу.
(Может только Совет)

tests/src/governance/technicalCommittee.test.ts Outdated Show resolved Hide resolved
tests/src/governance/technicalCommittee.test.ts Outdated Show resolved Hide resolved
tests/src/governance/technicalCommittee.test.ts Outdated Show resolved Hide resolved
runtime/common/config/pallets/governance/fellowship.rs Outdated Show resolved Hide resolved
runtime/common/config/pallets/governance/scheduler.rs Outdated Show resolved Hide resolved
runtime/common/construct_runtime.rs Outdated Show resolved Hide resolved
#[cfg(feature = "preimage")]
Preimage: pallet_preimage = 41,
#[cfg(feature = "preimage")]
Preimage: pallet_preimage::{Pallet, Call, Storage, Event<T>} = 41,
Copy link
Member

@CertainLach CertainLach Aug 18, 2023

Choose a reason for hiding this comment

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

Когда какие-то компоненты выбрасываются, стоит писать зачем и какие
+ в construct_runtime есть exclude_parts, чтобы не перечислять то что есть, но явно исключить то, что нам мешает (Тут я так вижу нам ничего не мешает, и просто перечислены все дефолт компоненты? Зачем тогда менялось описание у preimage?)

runtime/quartz/src/governance_timings.rs Show resolved Hide resolved
PraetorP and others added 7 commits August 18, 2023 12:04
Added fn for hard reset some of pallets
Added hard reset for some of the pallets
Co-authored-by: Yaroslav Bolyukin <iam@lach.pw>
- Removed debug test info
- Added new test for current Add\Remove assoc. type
- Changed gov settings
-Added helper `getMembers` for RankedCollective group
.github/workflows/gov.yml Outdated Show resolved Hide resolved
# Substrate Dependencies

# Note: `package = "parity-scale-codec"` must be supplied since the `Encode` macro searches for it.
codec = { workspace = true, package = "parity-scale-codec" }
Copy link
Member

Choose a reason for hiding this comment

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

Может, в новых палетах ссылаться через исходное название?

Suggested change
codec = { workspace = true, package = "parity-scale-codec" }
parity-scale-codec = { workspace = true }

Cargo начал жаловаться на атрибут package, потому что он тут не работает, и хотелось бы эти ворнинги постепенно везде исправить

Copy link
Member

Choose a reason for hiding this comment

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

Макросы фрейма ищут parity-scale-codec, они просто принимают его и в качестве названия, и в качестве пакета
И когда мы передаём в качестве названия пакета, нам не хватает указать это название в воркспейсе, макросы использующие Codec не умеют работать с воркспейсами

Copy link
Member

Choose a reason for hiding this comment

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

В репах субстрата и polkadot тоже потихоньку переходит

runtime/common/construct_runtime.rs Outdated Show resolved Hide resolved
runtime/unique/src/governance_timings.rs Outdated Show resolved Hide resolved
@CertainLach CertainLach merged commit 1a4aa2d into develop Aug 23, 2023
12 of 19 checks passed
@CertainLach CertainLach deleted the feature/rebase-democracy branch August 23, 2023 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-governance for CI workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants