[iip-15]sgdRegistry implementation #3845
Conversation
millken
requested review from
CoderZhi,
dustinxie,
Liuhaai and
a team
as code owners
Codecov Report
@@ Coverage Diff @@
## master #3845 +/- ##
==========================================
+ Coverage 75.38% 75.39% +0.01%
==========================================
Files 303 318 +15
Lines 25923 27297 +1374
==========================================
+ Hits 19541 20581 +1040
- Misses 5360 5660 +300
- Partials 1022 1056 +34
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
blockchain/config.go
Outdated
| @@ -32,6 +32,9 @@ type ( | |||
| BloomfilterIndexDBPath string `yaml:"bloomfilterIndexDBPath"` | |||
| CandidateIndexDBPath string `yaml:"candidateIndexDBPath"` | |||
| StakingIndexDBPath string `yaml:"stakingIndexDBPath"` | |||
| SGDIndexDBPath string `yaml:"sgdIndexDBPath"` | |||
| SGDIndexCacheSize int `yaml:"sgdIndexCacheSize"` | |||
| SGDPercentage uint64 `yaml:"sgdPercentage"` | |||
There was a problem hiding this comment.
no need for percentage, we'll have a tiered reward structure, like 50% for 100k volume, 60% for 200k volume, etc.
blockindex/sgd.go
Outdated
| ) | ||
|
|
||
| // NewSGDRegistry creates a new SGDIndexer | ||
| func NewSGDRegistry(kv db.KVStore, kvCache cache.LRUCache, percentage uint64) SGDIndexer { |
There was a problem hiding this comment.
as commented above, remove precentage
also remove kvCache, it can be generated inside here
you can use a builder config to pass in the parameters needed, refer to 02f8190#diff-c65dd5a657c73eb89458c69a82a25fc0993655650b3ad477136955c31e50e2bb
blockindex/sgd.go
Outdated
| actType: actType, | ||
| sender: sender, | ||
| contract: contract, | ||
| createTime: blk.Header.Timestamp(), |
blockindex/sgd.go
Outdated
| // CheckContract | ||
| CheckContract(contract string) (address.Address, uint64, bool) | ||
| // GetSGDIndex | ||
| GetSGDIndex(contract string) (*indexpb.SGDIndex, error) |
There was a problem hiding this comment.
no need to expose this func as part of interface?
There was a problem hiding this comment.
expose this func to get all the state of the contract, checkContract can only get receiver and percentage
blockindex/indexpb/index.proto
Outdated
|
|
||
| message SGDIndex { | ||
| string contract = 1; | ||
| string receiver = 2; |
There was a problem hiding this comment.
define these 2 as []byte, see comment below
| if err != nil { | ||
| return err | ||
| } | ||
| sgdIndex.Approved = true |
There was a problem hiding this comment.
should verify sgdIndex.Approved = false here?
depends on the contract's logic, can the same contract by approved twice? pls check it
There was a problem hiding this comment.
contract can only be approved once, otherwise will be reverted
blockindex/sgd_indexer.go
Outdated
| type ( | ||
| // SGDRegistry is the interface for Sharing of Gas-fee with DApps | ||
| SGDRegistry interface { | ||
| blockdao.BlockIndexer | ||
| // CheckContract returns the contract's eligibility for SGD and percentage | ||
| CheckContract(context.Context, string) (address.Address, uint64, bool, error) | ||
| // FetchContracts returns all contracts that are eligible for SGD | ||
| FetchContracts(context.Context) ([]*indexpb.SGDIndex, error) |
There was a problem hiding this comment.
should not use *indexpb.SGDIndex, which is storage format
There was a problem hiding this comment.
define a struct sgdIndex, and convert it to/from *indexpb.SGDIndex
There was a problem hiding this comment.
fixed, defined a SGDIndex
| if kv == nil { | ||
| panic("nil kvstore") | ||
| } | ||
| if contract != "" { |
There was a problem hiding this comment.
if contract == "" should also err out
There was a problem hiding this comment.
in dev or testnet env, it is allowed to be left blank when we not deployed contracts
| } | ||
| return nil, err | ||
| } | ||
| sgdIndexes := make([]*SGDIndex, 0, len(values)) |
There was a problem hiding this comment.
sgdIndexPb := indexpb.SGDIndex{}
move to here, can re-use the same var in the for loop below
blockchain/genesis/genesis.go
Outdated
| // SystemSGDContractAddress is the address of system sgd contract | ||
| SystemSGDContractAddress string `yaml:"sgdContractAddress"` | ||
| // SystemSGDContractHeight is the height of system sgd contract | ||
| SystemSGDContractHeight uint64 `yaml:"sgdContractHeight"` |
There was a problem hiding this comment.
yaml name should be same as var, with first letter change to small
|
SonarCloud Quality Gate failed.
|
Description
base on #3844 #3816
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: