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

Luxor/vested wg spending #5072

Merged
merged 40 commits into from
Apr 3, 2024

Conversation

ignazio-bovo
Copy link
Contributor

@ignazio-bovo ignazio-bovo commented Feb 5, 2024

Addresses: #4948
based on top of #5062

Note: I have used the extrinsic vested_transfer inside the extrinsic spend_from_budget all extrinsic in substrate are now transactional by default, this means that if the vested_transfer fails then spend_from_budget is equivalent to a no-op.
Also I have created one internal account for each WG where tokens are minted before they are transferred with vesting.

@ignazio-bovo
Copy link
Contributor Author

To be rebased onto #5062

@mnaamani
Copy link
Contributor

I think there is a simpler way to do this, without needing to have an account on each working group.
Instead of pay_from_budget into the working group account then doing vesting::vested_transfer you should be able to pay from the budget directly into the target account, then do vesting::force_vested_transfer with source and target being the final destination. The balances::transfer inside force_vested_transfer will be a successful no-op, and then the vesting schedule will be applied.

@mnaamani mnaamani self-requested a review March 16, 2024 22:38
@mnaamani
Copy link
Contributor

Also I have created one internal account for each WG where tokens are minted before they are transferred with vesting.

#5072 (comment)

@mnaamani
Copy link
Contributor

I think this now needs to be rebased on luxor since I merged the reduce council budget PR

Copy link
Contributor

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

some additional cleanup for build to works after refactor (removing the moduleId in working groups)

runtime-modules/content/src/tests/mock.rs Outdated Show resolved Hide resolved
runtime-modules/content/src/tests/mock.rs Outdated Show resolved Hide resolved
runtime/src/lib.rs Show resolved Hide resolved
@kdembler kdembler requested a review from mnaamani March 29, 2024 09:59
Copy link
Contributor

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

Benchmark is failing

Error: Input("Benchmark working_group::vested_spend_from_budget failed: AmountLow")
2024-03-29 22:54:25 Starting benchmark: working_group::vested_spend_from_budget    
There was a problem generating the weights for working_group, check the error above
Error: Process completed with exit code 1.

None
);

let amount_vesting = VestingBalanceOf::<T>::from(10_000u32);
Copy link
Contributor

@mnaamani mnaamani Mar 30, 2024

Choose a reason for hiding this comment

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

Benchmark is failing with error hereAmountLow because amount is less than vesting pallet configuration of <T as vesting::Config>::MinVestedTransfer::get()

Copy link
Contributor

Choose a reason for hiding this comment

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

So perhaps use that constant to make the amount_vesting at least that amount.

Copy link
Contributor

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

from earlier

None
);

let amount_vesting = VestingBalanceOf::<T>::from(10_000u32);
Copy link
Contributor

Choose a reason for hiding this comment

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

So perhaps use that constant to make the amount_vesting at least that amount.


let amount_vesting = VestingBalanceOf::<T>::from(10_000u32);
let budget = BalanceOf::<T>::from(100_000u32);
let block_no = 100u32;
Copy link
Contributor

Choose a reason for hiding this comment

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

block_no is not used

@mnaamani mnaamani self-requested a review April 2, 2024 17:34
@mnaamani
Copy link
Contributor

mnaamani commented Apr 3, 2024

strange that I'm seeing errors like ../../query-node/start.sh: line 11: docker-compose: command not found in CI workflow runs..
Has it been deprecated and we should replace all occurrences of that command with docker compose command?

I think you forgot to commit changes to: types/src/augment/augment-api-rpc.ts and types/src/augment/augment-types.ts

Copy link
Contributor

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

LGTM, next PR to luxor should commit changes to types/src/*.rs missed in this PR

Running integration tests locally on my machine works, we need to investigate why they are failing on github runners.

@mnaamani mnaamani merged commit aed16d0 into Joystream:luxor Apr 3, 2024
19 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants