feat: Cudos partial balance and delegations movement #387
Conversation
| return bondDenom, nil | ||
| } | ||
|
|
||
| func parseGenesisData(app *App, ctx sdk.Context, jsonData map[string]interface{}, genDoc *tmtypes.GenesisDoc, cudosCfg *CudosMergeConfig, manifest *UpgradeManifest) (*GenesisData, error) { |
There was a problem hiding this comment.
What was the motivation behind this changing passing from by-pointer to by-value? Was it attempt to disable theoreticall possibility of accidental modification of ctx instance value?
The sdk.Context is quite big structure, from performance perspective it would make sense to keep passibg by-pointer.
There was a problem hiding this comment.
It was just unification with other functions. I can unify it to convert everything to pointer.
There was a problem hiding this comment.
Actually some CosmoSDK function expects sdk.Context instead of *sdk.Context for example GetValidator(ctx sdk.Context
There was a problem hiding this comment.
Basically all CosmosSDK functions require sdk.Context, looks like this is intended way of passing.
| func convertAmount(app *App, ctx sdk.Context, genesisData *GenesisData, amount sdk.Int, cudosCfg *CudosMergeConfig) (sdk.Int, error) { | ||
| balance := sdk.NewCoins(sdk.NewCoin(genesisData.bondDenom, amount)) | ||
| convertedBalance, err := convertBalance(app, ctx, balance, cudosCfg) | ||
| if err != nil { | ||
| return sdk.ZeroInt(), err | ||
| } | ||
| return convertedBalance.AmountOf(app.StakingKeeper.BondDenom(ctx)), nil | ||
|
|
||
| } |
There was a problem hiding this comment.
Nit pick: Performance wise, this is quite hoop (it is called in the 2nd nested for loop. But I do see that it iwas obviously the easiest solution to achive what is necessary.
| srcCoins := sourceDelegations.MustGet(validatorAddr) | ||
| sourceDelegations, exists := genesisData.delegations.Get(fromDelegatorAddress) | ||
| if !exists { | ||
| return fmt.Errorf("genesis source delegations of %s not found", fromDelegatorAddress) |
There was a problem hiding this comment.
It kind of feels like this should not be an error case, but simply a valid case for doing nothing = simple success return from the function.
| return fmt.Errorf("genesis source delegations of %s not found", fromDelegatorAddress) | |
| return nil |
Though I see, the philosophical bent here - function is implemented the way that it is expected to be called with valid delegator & validator & amount delegation tripplet, and is expected to fail if any of these do not checkout against the situation on the ground ...
There was a problem hiding this comment.
Ignore the comment above, it is the philosophical bent - I checked as how the function is used
| destDelegation.Set(validatorAddr, destCoins.Add(srcCoins...)) | ||
| sourceAmount, exists := sourceDelegations.Get(validatorAddress) | ||
| if !exists { | ||
| return fmt.Errorf("genesis source delegation of %s to specific validator %s not found", fromDelegatorAddress, validatorAddress) |
There was a problem hiding this comment.
Analogical to this comment.
Though my sensation of valid philosophical bent here is starting to grow.
|
|
||
| if delegatedAmount.GT(remainingAmountToMove) { | ||
| if delegatedAmount.GTE(remainingAmountToMove) { |
There was a problem hiding this comment.
Shouldn't this be GT(...) rather than GTE(...), as it was riginally?
| func (om *OrderedMap[K, V]) GetOrSetDefault(key K, defaultValue V) (V, bool) { | ||
| if value, exists := om.Get(key); exists { | ||
| return value, false | ||
| } else { | ||
| om.Set(key, defaultValue) | ||
| return om.MustGet(key), true | ||
| } | ||
| } | ||
|
|
||
| // Iterate returns a channel that yields key-value pairs in insertion order | ||
| func (om *OrderedMap[K, V]) Iterate() <-chan Pair[K, V] { | ||
| ch := make(chan Pair[K, V]) | ||
| go func() { | ||
| for _, key := range om.keys { | ||
| ch <- Pair[K, V]{Key: key, Value: om.MustGet(key)} | ||
| } | ||
| close(ch) | ||
| }() | ||
| return ch | ||
| } |
There was a problem hiding this comment.
Keep in mind that this solution does a lot of shuffling data through the tread safe queueue (most probably using mutex) - internally the channel is implemented effectivelly as a thread safe queue.
This involves literally copying value of every single element of a collection the iteration goes through in to the channel (in this case copying every single Pair[K, V] value) over to channel queue ...
Looking at this solely from the performance perspective, it is not worth it.
👉 Though I do understand, that given the resulting performance impact in our use case, it is practivally irrelevant + this approach makes code more lean and so more readable ...
Proposed Changes
[describe the changes here...]
Linked Issues
[if applicable, add links to issues resolved by this PR]
Types of changes
What type of change does this pull request make (put an
xin the boxes that apply)?Checklist
Put an
xin the boxes that apply:If applicable
Further comments
[if this is a relatively large or complex change, kick off a discussion by explaining why you chose the solution you did, what alternatives you considered, etc...]