-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix: bugs discovered during simulation #31
fix: bugs discovered during simulation #31
Conversation
DoCancelProvideInsurance when cancel provide insurance, we should return all of its spendable coins from both derived address and fee pool address. DoWithdrawInsurance we can accept request only paired or unpaired insurances, not unpairing insurance. it because unpairing insurance is already in state transition situation at epoch, so its weird to queue the request for that insurance.
fix core logics before: * there can be bug because chunk's status is changed to unpairing but, current paired chunk's status is still Paired and chunk have paired insurance id even if it is unpairing chunk. after: if paired insurance of paired chunk have invalid insurance, then unpairing it and add it to out insurances to hande just like other unpairing chunks. add missing invariant checks * newly added RedelegationInfosInvariant was not included chore * refactor variables name in invariants.go * use lsm's own event key types, not other module's. * add module name to each event
todo: need to run it in local
TODO: Solve cycle problem
TODO: deal with staking module's initial stake part
…m/simulation-advance-epoch # Conflicts: # app/params/weights.go # proto/canto/liquidstaking/v1/liquidstaking.proto # x/liquidstaking/keeper/liquidstaking.go # x/liquidstaking/simulation/proposals.go # x/liquidstaking/spec/02_state.md # x/liquidstaking/spec/chunk_state_transition_diagram.png # x/liquidstaking/spec/insurance_state_transition_diagram.png # x/liquidstaking/types/liquidstaking.pb.go
del shares can be less than 250K (e.g. slashing during re-delegation period, not yet reached epoch)
now chunk invariants does not check its delegation shares. it can be changed by slashing during re-delegation.
decision must be based on current block time. if current block time >= info.CompletionTime, then it is matured. if not, it is un-matured.
disabled mimicking begin block logic of distribution module at epoch
target insurance must be pairing
must use validator retrieved from latest state.
EpochInfo.CurrentEpochStartHeight is set to the block height at the time of init genesis. This state is changeable when export and import, so exclude it.
when ins directs Unbonding validator, then this situation can happen. In this situation, we just unpair and undelegate it.
* fix dynamic fee rate as default
this is related with recent patch. outIns is already unpairing for unpairing_for_withdrawa, so we must not update it. chore: removed un-used method.
out insurance can be PAIRED status (ranked out but no replacement). if it is not unpairing or unpairing for withdrawal we must change its status to unpairing because unbonding will be started.
when decide unstakable chunk, we should consider whether it is already queued or not.
optimize gas consumption * added new internal state InsuranceState * when calc NetAmount we don't need insurance states. so separate that state which will be used when query. update sim * apply default genesis * don't allow param change * enable inflation module temp: apply sdk.DefaultPowerReduction when calc epoch provision. this will be changed at next PR
x/liquidstaking/keeper/grpc_query.go
Outdated
return &types.QueryStatesResponse{ | ||
NetAmountState: nas, | ||
InsuranceState: is, | ||
}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the endpoint corresponds to the API being used on the dashboard that has already been delivered to the frontend development team, it is not good to have a breaking change, so it will need to be reviewed was the endpoint already in use on the dashboard page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. Frontend team already applied formal endpoints in here.
How about creating a PR related with this change for them? I can do it and need no much time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary to change the response struct due to refactor for internal logic, it's better to update it at once when future modifications are inevitable
If they are already using it, use the internal getter function that has been refactored, but as a result, 'QueryStatesResponse' would be better to use the existing structure as it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added NetAmountStateEssentials
which is used by all core logics of lsm.
NetAmountState is used just for query and testing.
When call GetNetAmountState it calls GetNetAmountStateEssentials internally and use that result to create struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dongsam
If you are ok with changes, I'll update the spec too. SPEC is not updated yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that part is looks good. Let's continue with the power reduction part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated spec in here 04d03d3
app/sim_test.go
Outdated
@@ -308,6 +315,7 @@ func TestAppStateDeterminism(t *testing.T) { | |||
} | |||
|
|||
func TestAppSimulationAfterImport(t *testing.T) { | |||
liquidstakingtypes.ChunkSize = sdk.TokensFromConsensusPower(250_000, sdk.DefaultPowerReduction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part can be removed because it is already being done in init()
app/sim_test.go
Outdated
// {app.keys[erc20types.StoreKey], newApp.keys[erc20types.StoreKey], [][]byte{}}, | ||
//{app.keys[epochstypes.StoreKey], newApp.keys[epochstypes.StoreKey], [][]byte{}}, | ||
{app.keys[epochstypes.StoreKey], newApp.keys[epochstypes.StoreKey], [][]byte{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be commented, Ref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in https://github.com/Canto-Network/Canto/blob/main/app/sim_test.go#L218-L219
comment is written by myself but will use comment you linked when merge main branch.
app/params/weights.go
Outdated
DefaultWeightUpdateDynamicFeeRateProposal int = 2 | ||
DefaultWeightUpdateMaximumDiscountRate = 2 | ||
DefaultWeightAdvanceEpoch int = 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused weight variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in a9b2103
app/app.go
Outdated
baseKeeper := bankkeeper.NewBaseKeeper( | ||
appCodec, keys[banktypes.StoreKey], app.AccountKeeper, app.GetSubspace(banktypes.ModuleName), app.BlockedAddrs(), | ||
) | ||
app.BankKeeper = baseKeeper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason or intention to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can delete it. Done in 781a833.
@@ -42,6 +40,6 @@ func CalculateEpochMintProvision( | |||
// Multiply epochMintProvision with power reduction (10^18 for evmos) as the | |||
// calculation is based on `evmos` and the issued tokens need to be given in | |||
// `aevmos` | |||
epochProvision = epochProvision.Mul(ethermint.PowerReduction.ToDec()) | |||
epochProvision = epochProvision.Mul(sdk.DefaultPowerReduction.ToDec()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part needs to be reverted before the PR merges
more understandable and readable tc
epoch module's state can be changed when initGenesis.
all core logics using NetAmount before now use NetAmountEssentials. NetAmount is used just for querying.
In order to solve the power reduction issue I mentioned yesterday, I think the reduction modification in canto should be reverted to ethermint and revised from sdk simulation to big int |
Yeah definitely we need to do that. I started that work after requesting your review for NetAmountStateEssential part. During your review for updated codes, I'll handle that. |
for this, applied custom ibc-go and cosmos-sdk to updated initialStake as sdk.Int
Sure! I fixed it. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's proceed additionally with the gas optimization, NetAmountState structure, and customizing sdk simulation parts in separate issues after upstream rebase/merge
Fixes
RedelegationInfo
rinfo.CompletionTime
, then it should returntrue
but formal implementation will returnfalse
.mustPairChunkAndDelegate
, must use latest Validator state.Gas Optimization
SPEC
InsuranceState
Simulation
10^6
LiquidUnstake
andClaimDiscountedReward
)10^6
when calc epoch mint provisionDependency
Refactoring
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...