PSE Schedule and Distribution#14
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive periodic token distribution system for the PSE module. It adds functionality to mint tokens at genesis, distribute them to module accounts, and automatically transfer these tokens to multisig wallets on a monthly schedule over 84 months.
Key changes:
- Adds distribution schedule management with sub-account mappings for routing tokens
- Implements bootstrap functionality to mint and allocate tokens across 5 module accounts
- Adds automatic periodic distribution processing in EndBlock
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| x/pse/types/params.go | Adds module account constants, validation for sub-account mappings and distribution schedules |
| x/pse/types/params_test.go | Comprehensive test coverage for parameter validation logic |
| x/pse/keeper/bootstrap.go | Implements token minting and 84-month distribution schedule creation |
| x/pse/keeper/distribution.go | Core distribution processing logic executed in EndBlock |
| x/pse/keeper/genesis.go | Genesis import/export with pending queue rebuilding |
| x/pse/keeper/params.go | Governance-controlled sub-account mapping updates with referential integrity checks |
| x/pse/module.go | Integrates distribution processing into EndBlock |
| proto files | Protocol buffer definitions for distribution types and events |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Create distribution periods for 84 months | ||
| distributionPeriods := make([]types.DistributionPeriod, 0, TotalDistributionMonths) | ||
|
|
||
| for month := range TotalDistributionMonths { |
There was a problem hiding this comment.
[nitpick] Using range over an integer creates an iterator from 0 to n-1, which works correctly here but is less explicit than for month := 0; month < TotalDistributionMonths; month++. The current syntax may be unfamiliar to developers not using Go 1.22+. Consider using the traditional for-loop syntax for better clarity and wider compatibility.
| for month := range TotalDistributionMonths { | |
| for month := 0; month < TotalDistributionMonths; month++ { |
There was a problem hiding this comment.
I don't think this is a valid comment.
| // Transfer tokens - panic on error (invariant violation) | ||
| if err := k.bankKeeper.SendCoinsFromModuleToAccount( | ||
| ctx, | ||
| dist.ModuleAccount, | ||
| subAccountAddr, | ||
| coinsToDistribute, | ||
| ); err != nil { | ||
| panic(fmt.Sprintf("balance invariant violated for module '%s': %v", dist.ModuleAccount, err)) |
There was a problem hiding this comment.
The panic on SendCoins failure assumes this is always a balance invariant violation, but other errors (e.g., module account doesn't exist, invalid permissions) could also cause failures. Consider checking the specific error type or providing more detailed context about what went wrong, as this will help with debugging in production.
| // Transfer tokens - panic on error (invariant violation) | |
| if err := k.bankKeeper.SendCoinsFromModuleToAccount( | |
| ctx, | |
| dist.ModuleAccount, | |
| subAccountAddr, | |
| coinsToDistribute, | |
| ); err != nil { | |
| panic(fmt.Sprintf("balance invariant violated for module '%s': %v", dist.ModuleAccount, err)) | |
| // Transfer tokens - handle error with detailed context | |
| if err := k.bankKeeper.SendCoinsFromModuleToAccount( | |
| ctx, | |
| dist.ModuleAccount, | |
| subAccountAddr, | |
| coinsToDistribute, | |
| ); err != nil { | |
| return fmt.Errorf("failed to send coins from module '%s' to sub account '%s' (amount: %s): %w", dist.ModuleAccount, subAccount, coinsToDistribute.String(), err) |
There was a problem hiding this comment.
If such error happens, (any error in distribution) means the logic has a big problem and should panic. I removed this panic and added a to-do in the end blocker section to be decided.
Anyway, if the module account has not enough balance, or any io error should be considered as invariant violation, and should be prevented in the update distrbution proposal logic.
There was a problem hiding this comment.
ignore this comment, not super accurate.
| // Build a set of module accounts that are currently in the distribution schedule | ||
| requiredModules := make(map[string]bool) | ||
| for _, period := range params.DistributionSchedule { | ||
| for _, dist := range period.Distributions { | ||
| requiredModules[dist.ModuleAccount] = true | ||
| } | ||
| } |
There was a problem hiding this comment.
This validation checks all periods in the schedule every time mappings are updated. For an 84-period schedule with multiple distributions per period, this is O(n*m) where n=periods and m=distributions per period. Since this runs on every governance proposal to update mappings, consider caching the set of required modules or storing it as part of params validation if mappings updates are frequent relative to schedule updates.
| // Build a set of module accounts that are currently in the distribution schedule | |
| requiredModules := make(map[string]bool) | |
| for _, period := range params.DistributionSchedule { | |
| for _, dist := range period.Distributions { | |
| requiredModules[dist.ModuleAccount] = true | |
| } | |
| } | |
| // Cache the set of required module accounts referenced in the distribution schedule | |
| requiredModules := getRequiredModuleAccounts(params.DistributionSchedule) |
There was a problem hiding this comment.
Th proposal does not happen so many times, it is better to have this logic and highter distribution logic. Also caching is not a good way in blockchain i think
ysv
left a comment
There was a problem hiding this comment.
@ysv reviewed 6 of 26 files at r1, 1 of 4 files at r2, all commit messages.
Reviewable status: 7 of 26 files reviewed, 12 unresolved discussions (waiting on @masihyeganeh and @miladz68)
a discussion (no related file):
Partially reviewed. Posting first batch of comments
x/pse/keeper/bootstrap.go line 24 at r2 (raw file):
// BootstrapTotalMint is the total amount to mint during bootstrap // 100 billion tokens (in base denomination units). BootstrapTotalMint = 100_000_000_000
this is 100B ucore while we need 100B TX
100B TX = 10^6 * 10^8 ucore
x/pse/keeper/bootstrap.go line 72 at r2 (raw file):
// // Returns an error if minting, distribution, or schedule creation fails. func (k Keeper) PerformBootstrap(
does this also effect generated by cored genesis.json ?
if I run cored init ... will it generate genesis with pse module and all params inside ?
x/pse/keeper/bootstrap.go line 194 at r2 (raw file):
// createDistributionSchedule creates an 84-month distribution schedule based on module account balances. // Each month distributes 1/84th of each module account's balance.
100B / 84 = 1.19047...
not very beautiful number.
Maybe we should ask management if we should round monthly distribution to 1.2B
proto/tx/pse/v1/distribution.proto line 21 at r2 (raw file):
(cosmos_proto.scalar) = "cosmos.AddressString", (gogoproto.moretags) = "yaml:\"sub_account_address\"" ];
I'm not sure about the name.
Maybe receiver_account_address ?
same for message name Receiver/RecepientAccountMapping
Code quote:
// sub_account_address is the multisig wallet address that will receive the token distributions.
string sub_account_address = 2 [
(cosmos_proto.scalar) = "cosmos.AddressString",
(gogoproto.moretags) = "yaml:\"sub_account_address\""
];proto/tx/pse/v1/distribution.proto line 25 at r2 (raw file):
// ModuleDistribution defines the amount to be distributed from a specific module account. message ModuleDistribution {
I didn't fully get meaning of 'Module' word here. maybe we can come up with more informative name. AccountAmountMap / ModuleAccountAmountMap
proto/tx/pse/v1/distribution.proto line 43 at r2 (raw file):
// DistributionPeriod defines a single distribution event at a specific timestamp. // Multiple module accounts can distribute tokens at the same time. message DistributionPeriod {
again not sure about the name, maybe ScheduledDistribution is better option
proto/tx/pse/v1/distribution.proto line 47 at r2 (raw file):
uint64 distribution_time = 1 [ (gogoproto.moretags) = "yaml:\"distribution_time\"" ];
shouldn't we name this one scheduled_time also ? Doesn't it have the same meaning
Code quote:
uint64 distribution_time = 1 [
(gogoproto.moretags) = "yaml:\"distribution_time\""
];proto/tx/pse/v1/distribution.proto line 76 at r2 (raw file):
// actual_distribution_time is when the distribution actually occurred (Unix timestamp in seconds). uint64 actual_distribution_time = 4 [
nit: I'm not sure what is the naming convetion in cosmos and if it exists. But I really like to name timestamp realated attributes as {verb}_at E.g distributed_at here
| // Create distribution periods for 84 months | ||
| distributionPeriods := make([]types.DistributionPeriod, 0, TotalDistributionMonths) | ||
|
|
||
| for month := range TotalDistributionMonths { |
ysv
left a comment
There was a problem hiding this comment.
@ysv reviewed 1 of 26 files at r1, 1 of 4 files at r2.
Reviewable status: 9 of 26 files reviewed, 18 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @TxCorpi0x)
a discussion (no related file):
Previously, ysv (Yaroslav Savchuk) wrote…
Partially reviewed. Posting first batch of comments
Batch 2 published, still partially reviewed
proto/tx/pse/v1/distribution.proto line 21 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
I'm not sure about the name.
Maybereceiver_account_address?same for message name Receiver/RecepientAccountMapping
or ModuleRecepientAccountMapping
proto/tx/pse/v1/distribution.proto line 25 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
I didn't fully get meaning of 'Module' word here. maybe we can come up with more informative name. AccountAmountMap / ModuleAccountAmountMap
Or Allocation / ModuleAllocaiton
I prefer these options most ^
I thin we overuse distribution word
proto/tx/pse/v1/distribution.proto line 13 at r2 (raw file):
message SubAccountMapping { // module_account is the name of the module account holding the tokens to be distributed. string module_account = 1 [
nit: module_subaccount
proto/tx/pse/v1/distribution.proto line 50 at r2 (raw file):
// distributions is the list of amounts to distribute from each module account at this time. repeated ModuleDistribution distributions = 2 [
distributions -> allocations
x/pse/keeper/bootstrap.go line 196 at r2 (raw file):
// Each month distributes 1/84th of each module account's balance. // Uses proper Gregorian calendar calculation respecting all date rules including leap years. func (k Keeper) createDistributionSchedule(
I see clear division between of the functionality implemented in this file.
IMO we should separate schedule generation and interaction with cosmos SDK into separate functions (maybe files) and test this separately.
I don't feel confident with Current code
lets discuss on call
// get needed params
params, err := k.GetParams(ctx)
// build distribution schedule. No interactions with cosmos sdk keeper/store
distributionSchedule := buildDistributionSchedule()
// mint coins, send to module accounts
// store params etc.
x/pse/keeper/distribution.go line 30 at r2 (raw file):
// If earliest timestamp is in the future, nothing to do this block if timestamp > currentTimeUnix {
nit: variable used only once we can use sdkCtx.BlockTime() directly
Code quote:
currentTimeUnixx/pse/keeper/distribution.go line 265 at r2 (raw file):
// executeSingleDistribution executes a single module distribution. func (k Keeper) executeSingleDistribution(
executeSingleAllocation
x/pse/keeper/distribution.go line 322 at r2 (raw file):
} func (k Keeper) emitDistributionCompletedEvent(
nit: not sure if this is worth having in a separate method
ysv
left a comment
There was a problem hiding this comment.
@ysv reviewed 11 of 26 files at r1, 2 of 4 files at r2.
Reviewable status: 22 of 26 files reviewed, 20 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @TxCorpi0x)
x/pse/keeper/distribution.go line 380 at r2 (raw file):
// and completed distributions. This ensures the queue is in sync with actual state. // Used by InitGenesis and when schedule updates are made via governance. func (k Keeper) rebuildPendingQueue(ctx context.Context) error {
I can't understand the logic of this function.
Lets discuss
x/pse/types/errors.go line 22 at r2 (raw file):
ErrExportGenesis = sdkerrors.Register(ModuleName, 5, "error exporting genesis") // ErrDistributionScheduleNotFound is returned when a distribution schedule for a module account is not found.
do we really need that many errors ?
All this failures are possible from business perspective but can we somehow simplify
miladz68
left a comment
There was a problem hiding this comment.
@miladz68 reviewed 5 of 26 files at r1, 1 of 4 files at r2, all commit messages.
Reviewable status: 22 of 26 files reviewed, 40 unresolved discussions (waiting on @Copilot, @masihyeganeh, @TxCorpi0x, and @ysv)
proto/tx/pse/v1/distribution.proto line 21 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
or ModuleRecepientAccountMapping
second field will be called recipient_address
the strucute itself should be called ClearingAccountMapping
proto/tx/pse/v1/distribution.proto line 25 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
Or
Allocation/ModuleAllocaiton
I prefer these options most ^I thin we overuse distribution word
agreed on ClearingAccountAllocaiton
proto/tx/pse/v1/distribution.proto line 43 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
again not sure about the name, maybe ScheduledDistribution is better option
confirmed on the call.
proto/tx/pse/v1/distribution.proto line 47 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
shouldn't we name this one scheduled_time also ? Doesn't it have the same meaning
confirmed on call.
proto/tx/pse/v1/distribution.proto line 50 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
distributions -> allocations
agreed on call.
proto/tx/pse/v1/distribution.proto line 11 at r2 (raw file):
// SubAccountMapping defines the mapping between a module account and its sub account (multisig wallet). // This mapping can be modified via governance proposals. message SubAccountMapping {
we need to be able to update SubAccountMapping.
Is that commint is a differnt PR or task ?
proto/tx/pse/v1/distribution.proto line 27 at r2 (raw file):
message ModuleDistribution { // module_account is the name of the module account. string module_account = 1 [
ClearingAccount
proto/tx/pse/v1/distribution.proto line 58 at r2 (raw file):
// CompletedDistribution tracks a distribution that has been executed. // These are immutable historical records. message CompletedDistribution {
I think this type can be merged with Distribution period.
proto/tx/pse/v1/distribution.proto line 96 at r2 (raw file):
// PendingDistributionInfo provides detailed information about a scheduled pending distribution. // used for query purposes to get the details of a pending distribution. message PendingDistributionInfo {
agreed on call to remove it.
proto/tx/pse/v1/genesis.proto line 23 at r2 (raw file):
// pending_distribution_timestamps is a sorted list of timestamps that still need processing. // This will be rebuilt from the schedule during InitGenesis for consistency. repeated uint64 pending_distribution_timestamps = 3 [
shouldn't this contain the full distribution plan, not just timestamps ?
agreed to replace it with full distribution schedule.
x/pse/types/params.go line 11 at r2 (raw file):
const ( // ModuleAccountTreasury is the treasury module account name. ModuleAccountTreasury = "treasury"
prefix these module accounts with pse either pse_team or pse/team
x/pse/keeper/bootstrap.go line 20 at r2 (raw file):
// DefaultDistributionStartTime is the default start time for the distribution schedule // This is January 1, 2026, 00:00:00 UTC. DefaultDistributionStartTime = 1735689600
distributions will begin at the first day of the next month after upgrade.
x/pse/keeper/bootstrap.go line 24 at r2 (raw file):
// BootstrapTotalMint is the total amount to mint during bootstrap // 100 billion tokens (in base denomination units). BootstrapTotalMint = 100_000_000_000
we will min 100B tx token. But this is in utx and missing 6 zeros.
x/pse/keeper/bootstrap.go line 28 at r2 (raw file):
// BootstrapAllocation defines the initial allocation for a module account during bootstrap. type BootstrapAllocation struct {
seems to me that the bootstrap belongs to the upgrade module. also the name bootstrap could be improved (needs brain storming).
x/pse/keeper/bootstrap.go line 64 at r2 (raw file):
// a predefined time. This should be called during software upgrade. // // Parameters:
I don't think explaining the parameters is needed.
@ysv @masihyeganeh wdyt ?
x/pse/keeper/bootstrap.go line 72 at r2 (raw file):
// // Returns an error if minting, distribution, or schedule creation fails. func (k Keeper) PerformBootstrap(
we need a better name (possibly brain storming), maybe CreateInitialDistributionSchedule
x/pse/keeper/bootstrap.go line 72 at r2 (raw file):
// // Returns an error if minting, distribution, or schedule creation fails. func (k Keeper) PerformBootstrap(
break it down to creation of schedule and save of schedule.
move the creation part to upgrade handler and test separately.
x/pse/keeper/bootstrap.go line 119 at r2 (raw file):
// Calculate allocation amount allocationAmount := allocation.Percentage.MulInt(totalMintAmount).TruncateInt() if allocationAmount.IsZero() {
zero allocation is an error.
x/pse/keeper/bootstrap.go line 233 at r2 (raw file):
// Skip if amount is zero if monthlyAmount.IsZero() {
zero monthly amount is an error
x/pse/keeper/bootstrap.go line 251 at r2 (raw file):
// Add timestamp to pending timestamps queue if err := k.PendingTimestamps.Set(ctx, distributionTime); err != nil {
I am not fully understanding pending timestamps. we need to discuss.
proto/tx/pse/v1/params.proto line 30 at r2 (raw file):
// Must be sorted by distribution_time in ascending order. // Can be modified by governance proposals (only pending periods can be modified). repeated DistributionPeriod distribution_schedule = 3 [
possible discussion: maybe move this out of params field.
We agreed to move it out of params.
x/pse/keeper/bootstrap.go line 35 at r1 (raw file):
// DefaultBootstrapAllocations returns the default allocation percentages for module accounts. // These percentages should sum to 1.0 (100%). func DefaultBootstrapAllocations() []BootstrapAllocation {
here is final distribution for each module account.
proto/tx/pse/v1/event.proto line 24 at r2 (raw file):
]; // denom is the denomination of the tokens distributed. string denom = 6;
this is redundant, only bond denom is distributed.
proto/tx/pse/v1/event.proto line 26 at r2 (raw file):
string denom = 6; // block_height is the block height at which the distribution occurred. int64 block_height = 7;
this is redundant, block height is already part of the block.
x/pse/keeper/distribution.go line 163 at r2 (raw file):
bondDenom string subAccountMappings map[string]string // module_account -> sub_account periodByTimestamp map[uint64]types.DistributionPeriod
these maps are potential sources of non-determinsm.
| // Create distribution periods for 84 months | ||
| distributionPeriods := make([]types.DistributionPeriod, 0, TotalDistributionMonths) | ||
|
|
||
| for month := range TotalDistributionMonths { |
There was a problem hiding this comment.
I don't think this is a valid comment.
- Introduced new allocation schedule for PSE module, allowing for token distribution based on defined allocations. - Implemented bootstrap process to initialize allocations and mint tokens. - Updated related tests to validate the new allocation logic and ensure correct distribution behavior. - Refactored existing code to accommodate changes in allocation mapping and distribution processes.
- Refactored module account mappings to include new accounts: Community, Foundation, and Alliance. - Adjusted allocation percentages and distribution logic to accommodate the new account structure. - Implemented checks for excluded accounts to ensure they do not transfer funds to recipients during distribution. - Updated tests to reflect changes in account mappings and validate distribution behavior for excluded accounts.
TxCorpi0x
left a comment
There was a problem hiding this comment.
Reviewable status: 2 of 29 files reviewed, 37 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @ysv)
proto/tx/pse/v1/distribution.proto line 11 at r2 (raw file):
Previously, miladz68 (milad) wrote…
we need to be able to update SubAccountMapping.
Is that commint is a differnt PR or task ?
It would be in the next PR
proto/tx/pse/v1/distribution.proto line 13 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
nit: module_subaccount
Redesigned according to the call.
proto/tx/pse/v1/distribution.proto line 21 at r2 (raw file):
Previously, miladz68 (milad) wrote…
second field will be called
recipient_address
the strucute itself should be calledClearingAccountMapping
Done
proto/tx/pse/v1/distribution.proto line 25 at r2 (raw file):
Previously, miladz68 (milad) wrote…
agreed on
ClearingAccountAllocaiton
Done
proto/tx/pse/v1/distribution.proto line 27 at r2 (raw file):
Previously, miladz68 (milad) wrote…
ClearingAccount
Done
proto/tx/pse/v1/distribution.proto line 43 at r2 (raw file):
Previously, miladz68 (milad) wrote…
confirmed on the call.
Redesigned according to the call.
proto/tx/pse/v1/distribution.proto line 47 at r2 (raw file):
Previously, miladz68 (milad) wrote…
confirmed on call.
Redesigned according to the call.
proto/tx/pse/v1/distribution.proto line 50 at r2 (raw file):
Previously, miladz68 (milad) wrote…
agreed on call.
Redesigned according to the call.
proto/tx/pse/v1/distribution.proto line 58 at r2 (raw file):
Previously, miladz68 (milad) wrote…
I think this type can be merged with Distribution period.
Removed according to the call.
proto/tx/pse/v1/distribution.proto line 76 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
nit: I'm not sure what is the naming convetion in cosmos and if it exists. But I really like to name timestamp realated attributes as
{verb}_atE.gdistributed_athere
Done
proto/tx/pse/v1/distribution.proto line 96 at r2 (raw file):
Previously, miladz68 (milad) wrote…
agreed on call to remove it.
Done
proto/tx/pse/v1/event.proto line 24 at r2 (raw file):
Previously, miladz68 (milad) wrote…
this is redundant, only bond denom is distributed.
Done
proto/tx/pse/v1/event.proto line 26 at r2 (raw file):
Previously, miladz68 (milad) wrote…
this is redundant, block height is already part of the block.
Done
proto/tx/pse/v1/genesis.proto line 23 at r2 (raw file):
Previously, miladz68 (milad) wrote…
shouldn't this contain the full distribution plan, not just timestamps ?
agreed to replace it with full distribution schedule.
Redesigned according to the call.
proto/tx/pse/v1/params.proto line 30 at r2 (raw file):
Previously, miladz68 (milad) wrote…
possible discussion: maybe move this out of params field.
We agreed to move it out of params.
Redesigned according to the call.
x/pse/keeper/distribution.go line 30 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
nit: variable used only once we can use
sdkCtx.BlockTime()directly
Done
x/pse/keeper/distribution.go line 163 at r2 (raw file):
Previously, miladz68 (milad) wrote…
these maps are potential sources of non-determinsm.
The mapping is just to check the existence of a module account and its value so it won't have different results in different calls and sorting problem (non determinism), but it is also removed and redesigned according to the call
x/pse/keeper/distribution.go line 265 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
executeSingleAllocation
Redesigned according to the call.
x/pse/keeper/distribution.go line 322 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
nit: not sure if this is worth having in a separate method
Removed
x/pse/keeper/distribution.go line 380 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
I can't understand the logic of this function.
Lets discuss
Redesigned according to the call.
x/pse/types/errors.go line 22 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
do we really need that many errors ?
All this failures are possible from business perspective but can we somehow simplify
Cleaned Up.
x/pse/types/params.go line 11 at r2 (raw file):
Previously, miladz68 (milad) wrote…
prefix these module accounts with pse either
pse_teamorpse/team
Done
x/pse/keeper/bootstrap.go line 35 at r1 (raw file):
Previously, miladz68 (milad) wrote…
here is final distribution for each module account.
Module accounts replaced with these namings, and exclusions list added for (has community at the moment.)
x/pse/keeper/bootstrap.go line 20 at r2 (raw file):
Previously, miladz68 (milad) wrote…
distributions will begin at the first day of the next month after upgrade.
Set as 1st Dec 2025
x/pse/keeper/bootstrap.go line 24 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
this is 100B ucore while we need 100B TX
100B TX = 10^6 * 10^8 ucore
Done
x/pse/keeper/bootstrap.go line 24 at r2 (raw file):
Previously, miladz68 (milad) wrote…
we will min 100B tx token. But this is in utx and missing 6 zeros.
Done
x/pse/keeper/bootstrap.go line 28 at r2 (raw file):
Previously, miladz68 (milad) wrote…
seems to me that the bootstrap belongs to the upgrade module. also the name bootstrap could be improved (needs brain storming).
Moved to upgrade handler, but for the naming we did not discuss about it.
x/pse/keeper/bootstrap.go line 64 at r2 (raw file):
Previously, miladz68 (milad) wrote…
I don't think explaining the parameters is needed.
@ysv @masihyeganeh wdyt ?
Redesigned according to the call.
x/pse/keeper/bootstrap.go line 119 at r2 (raw file):
Previously, miladz68 (milad) wrote…
zero allocation is an error.
Done
x/pse/keeper/bootstrap.go line 196 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
I see clear division between of the functionality implemented in this file.
IMO we should separate schedule generation and interaction with cosmos SDK into separate functions (maybe files) and test this separately.I don't feel confident with Current code
lets discuss on call
// get needed params params, err := k.GetParams(ctx) // build distribution schedule. No interactions with cosmos sdk keeper/store distributionSchedule := buildDistributionSchedule() // mint coins, send to module accounts // store params etc.
Create and Save schedules defined in the distrbution.go and used in the bootstrapping in the upgrade handler
x/pse/keeper/bootstrap.go line 233 at r2 (raw file):
Previously, miladz68 (milad) wrote…
zero monthly amount is an error
Done
x/pse/keeper/bootstrap.go line 251 at r2 (raw file):
Previously, miladz68 (milad) wrote…
I am not fully understanding pending timestamps. we need to discuss.
Redesigned according to the call.
miladz68
left a comment
There was a problem hiding this comment.
@miladz68 reviewed 2 of 26 files at r1, 19 of 27 files at r3, 4 of 7 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 63 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)
x/pse/keeper/bootstrap.go line 72 at r2 (raw file):
Previously, miladz68 (milad) wrote…
break it down to creation of schedule and save of schedule.
move the creation part to upgrade handler and test separately.
closing this, since I left another comment on the new code.
x/pse/types/errors.go line 9 at r5 (raw file):
var ( // ErrInvalidAuthority is returned when the authority is invalid. ErrInvalidAuthority = sdkerrors.Register(ModuleName, 3, "invalid authority")
make error codes incremental starting from 2.
x/pse/keeper/params_test.go line 201 at r5 (raw file):
} func TestUpdateSubAccountMappings_ReferentialIntegrity(t *testing.T) {
since update subaccountmapping must always contains all module accounts, this tests will be redundant.
x/pse/keeper/params_test.go line 278 at r5 (raw file):
} func TestUpdateSubAccountMappings_Authority(t *testing.T) {
remove the subaccount name every in the code. since we decided to rename it.
proto/tx/pse/v1/genesis.proto line 16 at r5 (raw file):
// Field number 2 previously stored completed distributions and is now reserved. reserved 2;
why is it reserved ?
app/upgrade/v6/pse_init.go line 1 at r5 (raw file):
package v6
the upgrade handler should create clearing account mappings, the addresses must be provided by the management, but for now put krypton group as placeholder and leave a todo to upgrade to the values provided by management.
core17pmq7hp4upvmmveqexzuhzu64v36re3w3447n7dt46uwp594wtps97qlm5
app/upgrade/v6/pse_init.go line 17 at r5 (raw file):
// DefaultDistributionStartTime is the default start time for the distribution schedule // This is Dec 1, 2025, 00:00:00 UTC. DefaultDistributionStartTime = 1764547200
this should be calculated on the fly, as the first day of the next month after the upgrade.
@ysv what do you think ?
app/upgrade/v6/pse_init.go line 37 at r5 (raw file):
{ ModuleAccount: psetypes.ModuleAccountCommunity, Percentage: sdkmath.LegacyMustNewDecFromStr("0.40"), // 40% - not funded during initialization
it should be funded at initialization but must not partake in distribution, it should also not have a recipient address.
app/upgrade/v6/pse_init.go line 66 at r5 (raw file):
// how tokens will be gradually released over time from module accounts to recipients. // Should be called once during the software upgrade that introduces the PSE module. func InitPSEFundsAndSchedules(
InitPSEAllocationsAndSchedule
app/upgrade/v6/pse_init.go line 66 at r5 (raw file):
// how tokens will be gradually released over time from module accounts to recipients. // Should be called once during the software upgrade that introduces the PSE module. func InitPSEFundsAndSchedules(
This function should consist of 3 functions, CreateDistributionSchedule (in upgrade handler), SaveDistribtuionSchedule (in keeper), MintAndFundClearingAccounts (in upgrade handler).
app/upgrade/v6/pse_init.go line 93 at r5 (raw file):
// Step 1: Convert allocation percentages to absolute token amounts // Include all accounts (excluded accounts get schedules but won't transfer to recipients in EndBlock) moduleAccountBalances := make(map[string]sdkmath.Int)
this map will lead to non-deterministic behavior.
app/upgrade/v6/pse_init.go line 127 at r5 (raw file):
// Step 3: Generate the n-month distribution schedule // This defines when and how much each module account will distribute to recipients schedule, err := pskeeper.CreateDistributionSchedule(moduleAccountBalances, scheduleStartTime)
I think this functionCreateDistributionSchedule belongs to upgrade handler rather than keeper
x/pse/keeper/distribution_test.go line 22 at r5 (raw file):
// getAllocationSchedule returns the allocation schedule as a sorted list func getAllocationSchedule(ctx context.Context, pseKeeper keeper.Keeper, requireT *require.Assertions) []types.ScheduledDistribution {
I see this logic repeated in multiple places, let's move it to the keeper and re-use.
x/pse/keeper/distribution_test.go line 82 at r5 (raw file):
// Mint tokens to module accounts for distribution for moduleAccount, amount := range moduleBalances {
I think here you should use mint and fund function that I mentioned before.
x/pse/keeper/distribution_test.go line 157 at r5 (raw file):
// Get bond denom stakingParams, err := testApp.StakingKeeper.GetParams(ctx)
use stakingKeeper.BondDenom
x/pse/keeper/genesis.go line 17 at r5 (raw file):
// Clear any existing allocation schedule iter, err := k.AllocationSchedule.Iterate(ctx, nil)
I think the map must be empty at init genesis, so clearing the map seems redundant.
x/pse/keeper/genesis.go line 58 at r5 (raw file):
// Normalize nil slices to empty slices for consistent comparison if genesis.Params.ExcludedAddresses == nil {
you should normalize as part of DefaultGenesisState() and remove these and move them to that function if needs be.
x/pse/keeper/genesis.go line 61 at r5 (raw file):
genesis.Params.ExcludedAddresses = []string{} } if genesis.Params.ClearingAccountMappings == nil {
repeated comment:
move this normalization to the default function.
x/pse/keeper/genesis.go line 67 at r5 (raw file):
// Export allocation schedule from map to sorted list var allocationSchedule []types.ScheduledDistribution iter, err := k.AllocationSchedule.Iterate(ctx, nil)
create a function in keeper and reuse in here and other places.
x/pse/keeper/genesis.go line 82 at r5 (raw file):
// Sort by timestamp in ascending order sort.Slice(allocationSchedule, func(i, j int) bool {
just a repeat from previously, the objects must already be sorted by timestamp, if they are not, then that is a source of non-determinism.
x/pse/keeper/genesis.go line 89 at r5 (raw file):
// Normalize nil slice to empty slice if genesis.ScheduledDistributions == nil {
move this normalization to GetAllAllocationSchdedule function which you will create in keeper.
proto/tx/pse/v1/event.proto line 17 at r5 (raw file):
uint64 scheduled_at = 3; // distributed_at is the Unix timestamp when the distribution actually occurred. uint64 distributed_at = 4;
this is part of the block information as well. can be removed.
x/pse/keeper/params.go line 69 at r5 (raw file):
// UpdateSubAccountMappings updates the sub account mappings in params via governance. func (k Keeper) UpdateSubAccountMappings(
I think this should have a tx message to actually use it.
x/pse/keeper/params.go line 85 at r5 (raw file):
// Build a set of clearing accounts that are currently in the allocation schedule requiredAccounts := make(map[string]bool)
I am not super clear what is happening here, but requiredAccounts are a fixed set, you should not be able to add or remove and item, only update with a new address is allowed.
x/pse/keeper/params.go line 109 at r5 (raw file):
// Check that all required clearing accounts are present in the new mappings for clearingAccount := range requiredAccounts {
This is non-deterministic behavior.
x/pse/types/params.go line 42 at r5 (raw file):
// GetAllowedModuleAccounts returns a list of all allowed module account names. func GetAllowedModuleAccounts() []string {
is unsued.
x/pse/types/params.go line 60 at r5 (raw file):
func GetExcludedClearingAccounts() []string { return []string{ ModuleAccountCommunity, // Community allocation handled by separate feature
nit:
ModuleAccountCommunity is the only excluded module from distribution. it is safe to just check for it in IsExcludedClearingAccount function.
x/pse/types/params.go line 182 at r5 (raw file):
// Check for duplicate clearing accounts in the same period if seenClearingAccounts[alloc.ClearingAccount] {
you should check for an exact list match.
x/pse/types/params.go line 210 at r5 (raw file):
func ValidateScheduleMappingConsistency(schedule []ScheduledDistribution, mappings []ClearingAccountMapping) error { // Build a set of available clearing accounts from mappings availableAccounts := make(map[string]bool)
you should check for exact list element match.
x/pse/keeper/keeper.go line 88 at r5 (raw file):
// GetBondDenom returns the bond denomination from staking params. // This is used as the distribution denom for all PSE distributions. func (k Keeper) GetBondDenom(ctx sdk.Context) (string, error) {
you can call BondDenom on staking keeper to achieve this.
app/upgrade/v6/pse_init_test.go line 23 at r5 (raw file):
// getAllocationSchedule returns the allocation schedule as a sorted list func getAllocationSchedule(ctx context.Context, pseKeeper pskeeper.Keeper, requireT *require.Assertions) []psetypes.ScheduledDistribution {
correct me if I am wrong but I believe you need this function as part of pse keeper to use in export of genesis.
app/upgrade/v6/pse_init_test.go line 24 at r5 (raw file):
// getAllocationSchedule returns the allocation schedule as a sorted list func getAllocationSchedule(ctx context.Context, pseKeeper pskeeper.Keeper, requireT *require.Assertions) []psetypes.ScheduledDistribution { var schedule []psetypes.ScheduledDistribution
nit: I think list variables should be in plural form. suggestions schedules or scheduledDistributions
app/upgrade/v6/pse_init_test.go line 36 at r5 (raw file):
// Sort by timestamp sort.Slice(schedule, func(i, j int) bool {
it should already be sorted by timestamp. make a test to ensure that they are sorted by timestamp.
app/upgrade/v6/pse_init_test.go line 83 at r5 (raw file):
// Step 3: Verify module accounts have correct balances allocations := v6.DefaultAllocations() totalMintAmount := sdkmath.NewInt(v6.InitialTotalMint)
totalActualMint should be calculated and compared by diffing total supply before and after calling InitPSEFundsAndSchedules
app/upgrade/v6/pse_init_test.go line 104 at r5 (raw file):
// Step 5: Verify allocation schedule was created with n months allocationSchedule := getAllocationSchedule(ctx, pseKeeper, requireT) requireT.Len(allocationSchedule, psetypes.TotalAllocationMonths,
TotalAllocationMonths belongs to the upgrade module and types.
@ysv what do you think ?
app/upgrade/v6/pse_init_test.go line 110 at r5 (raw file):
requireT.Equal(uint64(v6.DefaultDistributionStartTime), allocationSchedule[0].Timestamp, "first period should start at default distribution start time") requireT.Greater(allocationSchedule[83].Timestamp, uint64(v6.DefaultDistributionStartTime),
you can assert that every distribution is at the first day of month and month number is one larger than previous.
app/upgrade/v6/pse_init_test.go line 113 at r5 (raw file):
"last period should be after start time") // Step 7: Verify each period has allocations for all 5 modules
I thins we have 6 module accounts.
app/upgrade/v6/pse_init_test.go line 138 at r5 (raw file):
} func TestCreateDistributionSchedule_MatchesInitialAllocations(t *testing.T) {
correct me if I am wrong, but I think this test is already included in the previous test ?
proto/tx/pse/v1/genesis.proto line 16 at r1 (raw file):
// completed_distributions is the immutable history of executed distributions. repeated CompletedDistribution completed_distributions = 2 [
if you are storing completed distributions, you should export/import them as well.
x/pse/keeper/distribution.go line 17 at r5 (raw file):
// Checks the earliest scheduled distribution and processes it if the current block time has passed its timestamp. // Only one distribution is processed per call. Should be called from EndBlock. func (k Keeper) ProcessClearingAccountDistributions(ctx context.Context) error {
naming suggestion: ProcessNextDistribution
@ysv what do you think ?
x/pse/keeper/distribution.go line 22 at r5 (raw file):
// Get bond denom from staking params //nolint:contextcheck // this is correct context passing bondDenom, err := k.GetBondDenom(sdkCtx)
move reading bond denom and params after the timestamp check. it will reduce the overhead since only once a month you need this variables read.
x/pse/keeper/distribution.go line 58 at r5 (raw file):
if timestamp > uint64(sdkCtx.BlockTime().Unix()) { return nil }
non-blocking suggestion:
abstract these lines into PeekNextAllocationSchedule which returns (AllocationSchedule,shouldProcess bool,error)
Code quote:
// Get iterator for the allocation schedule (sorted by timestamp ascending)
iter, err := k.AllocationSchedule.Iterate(ctx, nil)
if err != nil {
return err
}
// Return early if schedule is empty
if !iter.Valid() {
iter.Close()
return nil
}
// Extract the earliest scheduled distribution
kv, err := iter.KeyValue()
iter.Close()
if err != nil {
return err
}
timestamp := kv.Key
// Skip if distribution time has not yet arrived
// Since the map is sorted by timestamp, if the first item is in the future, all items are
if timestamp > uint64(sdkCtx.BlockTime().Unix()) {
return nil
}x/pse/keeper/distribution.go line 79 at r5 (raw file):
// Processes all allocations within a single scheduled distribution. // Any transfer failure indicates a state invariant violation (insufficient balance or invalid recipient). func (k Keeper) processScheduledAllocations(
suggestion for function name distributeAllocatedTokens
x/pse/keeper/distribution.go line 88 at r5 (raw file):
// Retrieve the scheduled distribution for this timestamp scheduledDistribution, err := k.AllocationSchedule.Get(ctx, timestamp)
you have already read the scheduled allocation in the iterator, pass it to the function so you are not reading it twice.
x/pse/keeper/distribution.go line 117 at r5 (raw file):
// Prepare coins to transfer coinsToAllocate := sdk.NewCoins(sdk.NewCoin(bondDenom, allocation.Amount))
coinsToSend or coinsToDistribute
x/pse/keeper/distribution.go line 181 at r5 (raw file):
allocations := make([]types.ClearingAccountAllocation, 0, len(moduleAccountBalances)) for clearingAccount, totalBalance := range moduleAccountBalances {
looks like non-deterministic behavior.
x/pse/keeper/distribution.go line 197 at r5 (raw file):
// Add this distribution period to the schedule if len(allocations) > 0 {
zero allocations is an error.
| // Transfer tokens - panic on error (invariant violation) | ||
| if err := k.bankKeeper.SendCoinsFromModuleToAccount( | ||
| ctx, | ||
| dist.ModuleAccount, | ||
| subAccountAddr, | ||
| coinsToDistribute, | ||
| ); err != nil { | ||
| panic(fmt.Sprintf("balance invariant violated for module '%s': %v", dist.ModuleAccount, err)) |
There was a problem hiding this comment.
ignore this comment, not super accurate.
miladz68
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 63 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)
x/pse/keeper/distribution.go line 17 at r5 (raw file):
Previously, miladz68 (milad) wrote…
naming suggestion:
ProcessNextDistribution
@ysv what do you think ?
agreed on the name.
app/upgrade/v6/pse_init.go line 17 at r5 (raw file):
Previously, miladz68 (milad) wrote…
this should be calculated on the fly, as the first day of the next month after the upgrade.
@ysv what do you think ?
let's keep it for now until further discussion.
miladz68
left a comment
There was a problem hiding this comment.
@miladz68 reviewed 4 of 7 files at r8, all commit messages.
Reviewable status: 26 of 29 files reviewed, 12 unresolved discussions (waiting on @Copilot, @masihyeganeh, @TxCorpi0x, and @ysv)
x/pse/keeper/params.go line 99 at r7 (raw file):
Previously, TxCorpi0x wrote…
This is a length check only, do we need to sort?
you are putting the list in the error log.
x/pse/keeper/params.go line 119 at r7 (raw file):
Previously, TxCorpi0x wrote…
This is a length check only, do we need to sort?
you are putting the list in the error log.
x/pse/types/params.go line 197 at r7 (raw file):
Previously, TxCorpi0x wrote…
this is for error return, do we need to sort it?
the error will be commited to the block and if it has different content, it will be AppHashMismatch.
miladz68
left a comment
There was a problem hiding this comment.
@miladz68 reviewed 1 of 7 files at r8.
Reviewable status: 27 of 29 files reviewed, 11 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)
miladz68
left a comment
There was a problem hiding this comment.
@miladz68 reviewed 2 of 7 files at r8.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)
TxCorpi0x
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @ysv)
x/pse/keeper/distribution.go line 102 at r5 (raw file):
Previously, miladz68 (milad) wrote…
@ysv that is the implementaion actually, the code skips community module from this distribution, which will be picked up by community distribution.
replace direct comparison with community clearing account
x/pse/keeper/distribution.go line 110 at r7 (raw file):
Previously, miladz68 (milad) wrote…
IsExcludedForAllocationname is not accurate. Community ClearingAccount is not excluded from allocation, just the distribution logic is different. probably should be IsCommunityAccount or ExcludedFromDirectDistribution.
@ysv you also mentioned that we should discuss it.
replace direct comparison with community clearing account
x/pse/keeper/params.go line 99 at r7 (raw file):
Previously, miladz68 (milad) wrote…
you are putting the list in the error log.
remove missing accounts from err message
x/pse/keeper/params.go line 119 at r7 (raw file):
Previously, miladz68 (milad) wrote…
you are putting the list in the error log.
remove extra accounts from err message
x/pse/types/params.go line 16 at r5 (raw file):
Previously, TxCorpi0x wrote…
This section is stating the name of the module accounts, clearing accounts act as an alias for these module accounts. as we should use these items in app wiring, I thought it is more meaningful here to have technical name,
Rename to Clearing Account
x/pse/types/params.go line 197 at r7 (raw file):
Previously, miladz68 (milad) wrote…
the error will be commited to the block and if it has different content, it will be AppHashMismatch.
remove expected amount from errors
x/pse/keeper/bootstrap.go line 72 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
does this also effect generated by cored genesis.json ?
if I runcored init ...will it generate genesis with pse module and all params inside ?
@ysv Please consider this one
x/pse/keeper/bootstrap.go line 194 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
100B / 84 = 1.19047...
not very beautiful number.
Maybe we should ask management if we should round monthly distribution to 1.2B
Some rounding is accepted.
TxCorpi0x
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @ysv)
proto/tx/pse/v1/event.proto line 9 at r5 (raw file):
Previously, TxCorpi0x wrote…
Done
Done
x/pse/keeper/distribution.go line 102 at r5 (raw file):
Previously, TxCorpi0x wrote…
replace direct comparison with community clearing account
Done
x/pse/keeper/distribution.go line 110 at r7 (raw file):
Previously, TxCorpi0x wrote…
replace direct comparison with community clearing account
Done
x/pse/keeper/params.go line 99 at r7 (raw file):
Previously, TxCorpi0x wrote…
remove missing accounts from err message
Done
x/pse/keeper/params.go line 119 at r7 (raw file):
Previously, TxCorpi0x wrote…
remove extra accounts from err message
Done
x/pse/types/params.go line 16 at r5 (raw file):
Previously, TxCorpi0x wrote…
Rename to Clearing Account
Done
x/pse/types/params.go line 197 at r7 (raw file):
Previously, TxCorpi0x wrote…
remove expected amount from errors
Done
ysv
left a comment
There was a problem hiding this comment.
@ysv reviewed 2 of 27 files at r3, 6 of 21 files at r6, 3 of 7 files at r7, 2 of 7 files at r8, 13 of 13 files at r9, 2 of 2 files at r11.
Reviewable status: 30 of 31 files reviewed, 9 unresolved discussions (waiting on @masihyeganeh and @miladz68)
a discussion (no related file):
myself: check non-community clearing account usage
app/upgrade/v6/pse_init.go line 207 at r11 (raw file):
// Step 2: Filter to only non-Community clearing accounts (for schedule and mappings) distributionAllocations := FilterNonCommunityAllocations(fundAllocations)
you can move step 2 into step 4 because you use this variable there for the first time.
And have 1 step less as result
Code quote:
// Step 2: Filter to only non-Community clearing accounts (for schedule and mappings)
distributionAllocations := FilterNonCommunityAllocations(fundAllocations)x/pse/types/params_test.go line 166 at r9 (raw file):
}, { name: "invalid_duplicate_module_account",
nit: clearing account
x/pse/keeper/distribution.go line 90 at r9 (raw file):
// Since the map is sorted by timestamp, if the first item is in the future, all items are currentTime := uint64(sdkCtx.BlockTime().Unix()) shouldProcess := timestamp <= currentTime
nit: don't need set these 2 variables.
You can merge into 1 or even put whole statement into return
Code quote:
currentTime := uint64(sdkCtx.BlockTime().Unix())
shouldProcess := timestamp <= currentTimeapp/upgrade/v6/pse_init.go line 74 at r10 (raw file):
var distributionAllocations []InitialFundAllocation for _, allocation := range fundAllocations { if allocation.ModuleAccount != psetypes.ClearingAccountCommunity {
I think ModuleAccount should be renamed to ClearingAccount everywhere
Code quote:
allocation.ModuleAccount
ysv
left a comment
There was a problem hiding this comment.
@ysv reviewed 13 of 13 files at r12, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @masihyeganeh and @miladz68)
a discussion (no related file):
Consistency of NonCommunityClearingAccounts slice
I can see that you have this code in a few places
for _, clearingAccount := range []string{
types.ClearingAccountFoundation,
types.ClearingAccountAlliance,
types.ClearingAccountPartnership,
types.ClearingAccountInvestors,
types.ClearingAccountTeam,
e.g distribution_test.go
However, you can replace it with the func GetNonCommunityClearingAccounts.
check this test and other places
I would also prefer to use it in func FilterNonCommunityAllocations(fundAllocations \[\]InitialFundAllocation) \[\]InitialFundAllocation { so we have consistent logic everywhere.
smth like this:
// FilterNonCommunityAllocations returns only the fund allocations for clearing accounts
// all of the clearing accounts except for Community.
func FilterNonCommunityAllocations(fundAllocations []InitialFundAllocation) []InitialFundAllocation {
var distributionAllocations []InitialFundAllocation
for _, allocation := range fundAllocations {
if lo.Contains(psetypes.GetNonCommunityClearingAccounts(), allocation.ModuleAccount) {
distributionAllocations = append(distributionAllocations, allocation)
}
}
return distributionAllocations
}
ysv
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @TxCorpi0x)
a discussion (no related file):
Previously, ysv (Yaroslav Savchuk) wrote…
Consistency of NonCommunityClearingAccounts slice
I can see that you have this code in a few places
for _, clearingAccount := range []string{ types.ClearingAccountFoundation, types.ClearingAccountAlliance, types.ClearingAccountPartnership, types.ClearingAccountInvestors, types.ClearingAccountTeam,e.g distribution_test.go
However, you can replace it with the
func GetNonCommunityClearingAccounts.
check this test and other places
I would also prefer to use it in
func FilterNonCommunityAllocations(fundAllocations \[\]InitialFundAllocation) \[\]InitialFundAllocation {so we have consistent logic everywhere.smth like this:
// FilterNonCommunityAllocations returns only the fund allocations for clearing accounts // all of the clearing accounts except for Community. func FilterNonCommunityAllocations(fundAllocations []InitialFundAllocation) []InitialFundAllocation { var distributionAllocations []InitialFundAllocation for _, allocation := range fundAllocations { if lo.Contains(psetypes.GetNonCommunityClearingAccounts(), allocation.ModuleAccount) { distributionAllocations = append(distributionAllocations, allocation) } } return distributionAllocations }
Let make sure we always to have a single source of truth for NonCommunityClearingAccounts.
Search in code by non-community nonCommunity etc.
Screenshot 2025-11-11 at 17.34.15
miladz68
left a comment
There was a problem hiding this comment.
@miladz68 reviewed 5 of 13 files at r9, 13 of 13 files at r12, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @masihyeganeh and @TxCorpi0x)
x/pse/types/params.go line 197 at r7 (raw file):
Previously, TxCorpi0x wrote…
Done
expectedAccount is stil being appended to the error message.
x/pse/types/params.go line 226 at r12 (raw file):
return errors.Wrapf( ErrInvalidParam, "period %d, allocation %d: no recipient mapping found for clearing account '%s'",
please ensure determinisim here as well.
x/pse/types/params.go line 176 at r8 (raw file):
// Check for duplicate clearing accounts in the same period if seenClearingAccounts[alloc.ClearingAccount] { return fmt.Errorf("period %d: duplicate clearing account %s in same period", i, alloc.ClearingAccount)
clearing account is still being appended to error message.
x/pse/keeper/distribute.go line 89 at r12 (raw file):
} // send leftover to community clearing account.
send leftover to CommunityPool.
Code quote:
send leftover to community clearing account.app/upgrade/v6/pse_init.go line 164 at r12 (raw file):
default: return nil, fmt.Errorf("unknown chain id: %s", chainID)
use "github.com/pkg/errors" or the sdk errors package for new err creation.
app/upgrade/v6/pse_init.go line 225 at r12 (raw file):
// Step 4: Generate the n-month distribution schedule (only for non-Community clearing accounts) // This defines when and how much each non-Community clearing account will distribute to recipients schedule, err := CreateDistributionSchedule(distributionAllocations, totalMintAmount, scheduleStartTime)
you are creating distribution schedule with community module excluded, I think that is a mistake.
x/pse/keeper/distribute.go line 69 at r12 (raw file):
// leftover is the amount of pse coin that is not distributed to any delegator. // It will be sent to community module account.
It will be sent to CommunityPool.
x/pse/keeper/distribute.go line 219 at r12 (raw file):
for _, delegation := range delegations { // NOTE: this division will have rounding errors up to 1 subunit, which is acceptable and will be ignored. // the sum of all rounding errors will be sent to community module account.
// the sum of all rounding errors will be sent to CommunityPool
Code quote:
// the sum of all rounding errors will be sent to community module account
TxCorpi0x
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @ysv)
a discussion (no related file):
Previously, ysv (Yaroslav Savchuk) wrote…
Let make sure we always to have a single source of truth for
NonCommunityClearingAccounts.
Search in code bynon-communitynonCommunityetc.
Screenshot 2025-11-11 at 17.34.15
Done
x/pse/keeper/distribute.go line 69 at r12 (raw file):
Previously, miladz68 (milad) wrote…
It will be sent to CommunityPool.
Done.
x/pse/keeper/distribute.go line 89 at r12 (raw file):
Previously, miladz68 (milad) wrote…
send leftover to CommunityPool.
Done.
x/pse/keeper/distribute.go line 219 at r12 (raw file):
Previously, miladz68 (milad) wrote…
// the sum of all rounding errors will be sent to CommunityPool
Done.
x/pse/types/params.go line 197 at r7 (raw file):
Previously, miladz68 (milad) wrote…
expectedAccount is stil being appended to the error message.
Errors are not part of consensus, so there is no problem for that, but is will remove it
x/pse/types/params.go line 176 at r8 (raw file):
Previously, miladz68 (milad) wrote…
clearing account is still being appended to error message.
Errors are not part of consensus, so there is no problem for that, moreover, allocations are sorted,
but is will remove it,
Note: determinism is important for whatever that changes the state.
x/pse/types/params.go line 226 at r12 (raw file):
Previously, miladz68 (milad) wrote…
please ensure determinisim here as well.
Errors are not part of consensus, so there is no problem for that, but is will remove it
app/upgrade/v6/pse_init.go line 74 at r10 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
I think ModuleAccount should be renamed to ClearingAccount everywhere
Done.
app/upgrade/v6/pse_init.go line 207 at r11 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
you can move step 2 into step 4 because you use this variable there for the first time.
And have 1 step less as result
Done.
app/upgrade/v6/pse_init.go line 164 at r12 (raw file):
Previously, miladz68 (milad) wrote…
use
"github.com/pkg/errors"or the sdk errors package for new err creation.
Done.
app/upgrade/v6/pse_init.go line 225 at r12 (raw file):
Previously, miladz68 (milad) wrote…
you are creating distribution schedule with community module excluded, I think that is a mistake.
Community should be included in mint, and excluded in distribution calculation, isn't it true?
miladz68
left a comment
There was a problem hiding this comment.
@miladz68 reviewed 9 of 9 files at r13, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)
app/upgrade/v6/pse_init.go line 164 at r12 (raw file):
Previously, TxCorpi0x wrote…
Done.
I meant to not use fmt package for error creation.
app/upgrade/v6/pse_init.go line 225 at r12 (raw file):
Previously, TxCorpi0x wrote…
Community should be included in mint, and excluded in distribution calculation, isn't it true?
included in mint and fund and will have a different distribution logic for monthly distribution.
miladz68
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)
x/pse/types/params.go line 226 at r12 (raw file):
Previously, TxCorpi0x wrote…
Errors are not part of consensus, so there is no problem for that, but is will remove it
let's discuss as requested by yaroslav.
miladz68
left a comment
There was a problem hiding this comment.
@miladz68 reviewed 3 of 3 files at r14, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)
TxCorpi0x
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @ysv)
app/upgrade/v6/pse_init.go line 164 at r12 (raw file):
Previously, miladz68 (milad) wrote…
I meant to not use fmt package for error creation.
Done.
app/upgrade/v6/pse_init.go line 225 at r12 (raw file):
Previously, miladz68 (milad) wrote…
included in mint and fund and will have a different distribution logic for monthly distribution.
Done.
miladz68
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh and @ysv)
miladz68
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)
miladz68
left a comment
There was a problem hiding this comment.
@miladz68 reviewed 3 of 4 files at r15, all commit messages.
Reviewable status: 30 of 31 files reviewed, 2 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)
ysv
left a comment
There was a problem hiding this comment.
@ysv reviewed 4 of 9 files at r13, 1 of 3 files at r14, 2 of 4 files at r15, 6 of 6 files at r16, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh and @TxCorpi0x)
miladz68
left a comment
There was a problem hiding this comment.
@miladz68 reviewed 6 of 6 files at r16, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh)
Description
This pull request introduces and documents a new periodic distribution system for the PSE module, including new protobuf definitions, updates to the genesis state, and integration of module account permissions. The changes add support for scheduled token distributions from module accounts to sub-accounts (multisig wallets), tracking of completed and pending distributions, and the ability to configure schedules and mappings via governance.
PSE Periodic Distribution System
Protobuf definitions and API documentation:
Added new protobuf files
distribution.protoandevent.protofor the PSE module, defining messages for sub-account mappings, module distributions, distribution periods, completed and pending distributions, and distribution-completed events. These enable structured scheduling, execution, and tracking of periodic token distributions.Updated
params.protoandgenesis.prototo include new fields for sub-account mappings, distribution schedules, completed distributions, and pending distribution timestamps in the module's parameters and genesis state.Expanded the API documentation (
docs/api.md) to describe the new types and fields, ensuring clarity for integrators and developers.App integration and permissions:
maccPerms) are initialized inapp.goto allow dynamic inclusion of PSE module accounts and their permissions, ensuring proper access control for distribution operations.app.goto injectAccountKeeperandBankKeeper, which are required for managing and executing token distributions.Reviewers checklist:
Authors checklist
This change is