[IIP-13] implement liquid staking indexer #3853
Conversation
blockindex/liquidstaking_indexer.go
Outdated
| blockdao.BlockIndexer | ||
|
|
||
| CandidateVotes(candidate address.Address) *big.Int | ||
| Buckets() ([]*Bucket, error) |
There was a problem hiding this comment.
what buckets are this interface returned? unknown from the naming
There was a problem hiding this comment.
ContractStakingBucket maybe better
There was a problem hiding this comment.
sorry, I mean the func name is blurry
| if err := s.kvstore.Stop(ctx); err != nil { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
should the cache be reset here?
There was a problem hiding this comment.
strictly speaking, it is recommended to reset the cache upon Stop to ensure proper functioning when it is re-Start, even though it is not actually utilized in that manner.
blockindex/liquidstaking_indexer.go
Outdated
| // new dirty cache for this block | ||
| // it's not necessary to use thread safe cache here, because only one thread will call this function | ||
| // and no update to cache will happen before dirty merge to clean | ||
| dirty := newLiquidStakingDirty(s.cache.cache) |
There was a problem hiding this comment.
s.cache.unsafe() may be better
blockindex/liquidstaking_indexer.go
Outdated
| if receipt.Status != uint64(iotextypes.ReceiptStatus_Success) { | ||
| continue | ||
| } | ||
| act, ok := actionMap[receipt.ActionHash] |
There was a problem hiding this comment.
@dustinxie are actions and receipts in order in the block. Do we need a hashmap to get the receipt of the tx
There was a problem hiding this comment.
These codes are no longer needed since the removal of the unused 'act' argument.
| func (s *liquidStakingIndexer) Buckets() ([]*Bucket, error) { | ||
| vbs := []*Bucket{} | ||
| for id, bi := range s.cache.getAllBucketInfo() { | ||
| bt := s.cache.mustGetBucketType(bi.TypeIndex) |
There was a problem hiding this comment.
The race condition isn't solved by a threadsafe map. The issue still exists if Putblock() is executed when another thread is running in the loop.
There was a problem hiding this comment.
pls check whether a mutex is necessary for liquidStakingIndexer class
There was a problem hiding this comment.
Indeed, multiple cache reads before and after the commit still require locking.
There was a problem hiding this comment.
The improvement looks good for me. But one mutex for this module is enough
There was a problem hiding this comment.
removed the mutex in cache
blockindex/liquidstaking_indexer.go
Outdated
| return s.cache.getActiveBucketType(), nil | ||
| } | ||
|
|
||
| func (s *liquidStakingIndexer) handleEvent(ctx context.Context, dirty *liquidStakingDirty, blk *block.Block, act *action.SealedEnvelope, log *action.Log) error { |
There was a problem hiding this comment.
the purpose of act as one of the arguments?
There was a problem hiding this comment.
act has been an unused argument and should be removed
| case "Transfer": | ||
| return dirty.handleTransferEvent(event) | ||
| default: | ||
| return nil |
There was a problem hiding this comment.
should inconsistency be ignored here?
There was a problem hiding this comment.
good question, events that do not require handling should be clearly labeled and return nil, while unexpected events should return error.
blockindex/liquidstaking_indexer.go
Outdated
| if err := s.kvstore.WriteBatch(batch); err != nil { | ||
| return err | ||
| } | ||
| if err := s.cache.merge(delta); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
sure, i think two changes need to do:
- move
cache.mergebeforekvstore.WriteBatch reloadCachewhen error occured
| Index uint64 | ||
| Candidate address.Address | ||
| Owner address.Address |
There was a problem hiding this comment.
question: different values with those in bucketinfo?
There was a problem hiding this comment.
the ContractStakingBucket is corresponding to staking.VoteBucket;
and type ContractStakingBucket = staking.VoteBucket will be added in pr 3860
|
|
||
| type ( | ||
| // ContractStakingIndexer is the interface of contract staking indexer | ||
| ContractStakingIndexer interface { |
There was a problem hiding this comment.
add liquidstaking_indexer_test.go for ContractStakingIndexer?
There was a problem hiding this comment.
already added tests in e2etest/liquid_staking_test.go
There was a problem hiding this comment.
also added unittests in liquidstaking_indexer_test.go
|
SonarCloud Quality Gate failed.
|
Description
This pr implements a new indexer that enables the querying of liquid staking state natively. This is a necessary step before combining native staking with liquid staking. The new indexer includes:
Clickup-task
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: