[Supplier] Implement supplier service activation #707
Conversation
…eat/delay-supplier-activation
red-0ne
added
supplier
|
|
||
| return nil | ||
| } | ||
|
|
||
| // getNextSessionStartHeight returns the current height's next session start height. | ||
| func getNextSessionStartHeight(ctx context.Context, sharedParams *sharedtypes.Params) int64 { |
There was a problem hiding this comment.
OPTIONAL NIT: Wdyt of beginning to have functions like this be receiver functions of sharedtypes.Params so they're easier to reuse?
There was a problem hiding this comment.
I'd rather have it at x/shared/session.go and pass in the query height.
The current getNextSessionStartHeight uses sdkCtx to get the current height which makes it non-portable to non on-chain use cases.
GetNextSessionStartHeight(sharedParams *sharedtypes.Params, queryHeight int64) int64
| @@ -155,3 +155,9 @@ func GetEarliestSupplierProofCommitHeight( | |||
| //return proofWindowOpenHeight + randCreateProofHeightOffset | |||
| return proofWindowOpenHeight | |||
| } | |||
|
|
|||
| // GetNextSessionStartHeight returns the start block height of the session | |||
|
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 707) |
WalkthroughThis update introduces significant enhancements across various components, primarily focusing on the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Outside diff range, codebase verification and nitpick comments (2)
x/session/keeper/session_hydrator.go (1)
171-171: FunctionhydrateSessionSuppliersis not integrated into the main application code.The function
hydrateSessionSuppliersis only referenced within its own file,session_hydrator.go, and does not appear to be utilized in the broader application logic.
- Ensure that the function is properly called and integrated within the main application code.
Analysis chain
LGTM! Verify the function integration.
The modification improves the validation of supplier activity.
However, ensure that the function
hydrateSessionSuppliersis properly integrated and executed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the function `hydrateSessionSuppliers` is properly integrated and executed. # Test: Search for the function usage. Expect: Proper integration in the codebase. rg --type go -A 5 $'hydrateSessionSuppliers'Length of output: 1194
Script:
#!/bin/bash # Description: Verify the usage of `session_hydrator.go` or its functions in the broader codebase. # Test: Search for references to `session_hydrator` or any functions within the file. rg --type go -A 5 'session_hydrator' | grep -v 'session_hydrator.go'Length of output: 1115
Script:
#!/bin/bash # Description: Verify the usage of `hydrateSessionSuppliers` function or `session_hydrator` struct in the main application code. # Test: Search for references to `hydrateSessionSuppliers` or `session_hydrator` in the main application code. rg --type go -A 5 'hydrateSessionSuppliers|session_hydrator' | grep -v 'test'Length of output: 1234
api/poktroll/shared/supplier.pulsar.go (1)
1016-1019: Document the new fieldServicesActivationHeightsMap.The new field
ServicesActivationHeightsMapshould be documented clearly to explain its purpose and usage.// services_activation_heights_map is a map of serviceIds to the height at // which the staked supplier will become active for that service. // Activation heights are session start heights. ServicesActivationHeightsMap map[string]uint64 `protobuf:"bytes,5,rep,name=services_activation_heights_map,json=servicesActivationHeightsMap,proto3" json:"services_activation_heights_map,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"varint,2,opt,name=value,proto3"`
| type _Supplier_5_map struct { | ||
| m *map[string]uint64 | ||
| } |
There was a problem hiding this comment.
Ensure proper initialization of the map.
The _Supplier_5_map struct should ensure the map is properly initialized to avoid nil pointer dereferences.
type _Supplier_5_map struct {
m *map[string]uint64
}
func new_Supplier_5_map() *_Supplier_5_map {
m := make(map[string]uint64)
return &_Supplier_5_map{m: &m}
}| func (x *_Supplier_5_map) Len() int { | ||
| if x.m == nil { | ||
| return 0 | ||
| } | ||
| return len(*x.m) | ||
| } |
There was a problem hiding this comment.
Check for nil map before accessing length.
The Len method should handle the case where the map is nil.
func (x *_Supplier_5_map) Len() int {
if x.m == nil {
return 0
}
return len(*x.m)
}| func (x *_Supplier_5_map) Range(f func(protoreflect.MapKey, protoreflect.Value) bool) { | ||
| if x.m == nil { | ||
| return | ||
| } | ||
| for k, v := range *x.m { | ||
| mapKey := (protoreflect.MapKey)(protoreflect.ValueOfString(k)) | ||
| mapValue := protoreflect.ValueOfUint64(v) | ||
| if !f(mapKey, mapValue) { | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Check for nil map before ranging.
The Range method should handle the case where the map is nil.
func (x *_Supplier_5_map) Range(f func(protoreflect.MapKey, protoreflect.Value) bool) {
if x.m == nil {
return
}
for k, v := range *x.m {
mapKey := (protoreflect.MapKey)(protoreflect.ValueOfString(k))
mapValue := protoreflect.ValueOfUint64(v)
if !f(mapKey, mapValue) {
break
}
}
}| func (x *_Supplier_5_map) Has(key protoreflect.MapKey) bool { | ||
| if x.m == nil { | ||
| return false | ||
| } | ||
| keyUnwrapped := key.String() | ||
| concreteValue := keyUnwrapped | ||
| _, ok := (*x.m)[concreteValue] | ||
| return ok | ||
| } |
There was a problem hiding this comment.
Check for nil map before checking key existence.
The Has method should handle the case where the map is nil.
func (x *_Supplier_5_map) Has(key protoreflect.MapKey) bool {
if x.m == nil {
return false
}
keyUnwrapped := key.String()
concreteValue := keyUnwrapped
_, ok := (*x.m)[concreteValue]
return ok
}| func (x *_Supplier_5_map) Clear(key protoreflect.MapKey) { | ||
| if x.m == nil { | ||
| return | ||
| } | ||
| keyUnwrapped := key.String() | ||
| concreteKey := keyUnwrapped | ||
| delete(*x.m, concreteKey) | ||
| } |
There was a problem hiding this comment.
Check for nil map before deleting a key.
The Clear method should handle the case where the map is nil.
func (x *_Supplier_5_map) Clear(key protoreflect.MapKey) {
if x.m == nil {
return
}
keyUnwrapped := key.String()
concreteKey := keyUnwrapped
delete(*x.m, concreteKey)
}| func (x *_Supplier_5_map) Set(key protoreflect.MapKey, value protoreflect.Value) { | ||
| if !key.IsValid() || !value.IsValid() { | ||
| panic("invalid key or value provided") | ||
| } | ||
| keyUnwrapped := key.String() | ||
| concreteKey := keyUnwrapped | ||
| valueUnwrapped := value.Uint() | ||
| concreteValue := valueUnwrapped | ||
| (*x.m)[concreteKey] = concreteValue | ||
| } |
There was a problem hiding this comment.
Handle invalid key or value in Set method.
The Set method should handle invalid keys or values gracefully.
func (x *_Supplier_5_map) Set(key protoreflect.MapKey, value protoreflect.Value) {
if !key.IsValid() || !value.IsValid() {
panic("invalid key or value provided")
}
keyUnwrapped := key.String()
concreteKey := keyUnwrapped
valueUnwrapped := value.Uint()
concreteValue := valueUnwrapped
(*x.m)[concreteKey] = concreteValue
}| func (x *_Supplier_5_map) Mutable(key protoreflect.MapKey) protoreflect.Value { | ||
| panic("should not call Mutable on protoreflect.Map whose value is not of type protoreflect.Message") | ||
| } |
There was a problem hiding this comment.
Avoid panic in Mutable method.
The Mutable method should avoid panicking and provide a meaningful error message.
func (x *_Supplier_5_map) Mutable(key protoreflect.MapKey) protoreflect.Value {
panic("should not call Mutable on protoreflect.Map whose value is not of type protoreflect.Message")
}| func (x *_Supplier_5_map) NewValue() protoreflect.Value { | ||
| v := uint64(0) | ||
| return protoreflect.ValueOfUint64(v) | ||
| } |
There was a problem hiding this comment.
Initialize new map value correctly.
The NewValue method should initialize a new map value correctly.
func (x *_Supplier_5_map) NewValue() protoreflect.Value {
v := uint64(0)
return protoreflect.ValueOfUint64(v)
}| func (x *_Supplier_5_map) IsValid() bool { | ||
| return x.m != nil | ||
| } |
There was a problem hiding this comment.
Check for nil map in IsValid method.
The IsValid method should handle the case where the map is nil.
func (x *_Supplier_5_map) IsValid() bool {
return x.m != nil
}| func (x *Supplier) GetServicesActivationHeightsMap() map[string]uint64 { | ||
| if x != nil { | ||
| return x.ServicesActivationHeightsMap | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Check for nil map in GetServicesActivationHeightsMap method.
The GetServicesActivationHeightsMap method should handle the case where the map is nil.
func (x *Supplier) GetServicesActivationHeightsMap() map[string]uint64 {
if x != nil {
return x.ServicesActivationHeightsMap
}
return nil
}Co-authored-by: Daniel Olshansky <[email protected]>
Summary
This PR modifies the functionality so that suppliers staking for services mid-session will become active and be included in
Session.Suppliersstarting from the next session.It adds the property
ServiceActivationHeightwhich is a map ofserviceIdtoactivationHeightto theSuppliertype and supports activation for both staking and restaking.A remaining issue, is the immediate effect of removing a service when a Supplier restakes.
~550 LOC are protobuf generated code
Issue
When a
Supplierstakes or restakes mid-session, the session endpoint may include that supplier before the next session starts. This can lead to inconsistencies between session queries and clients that fetchSessiondata only when a new session begins.Type of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_testmake test_e2ePR Testing (only if making code changes)
devnet-test-e2elabel to the PR.make trigger_ciif you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores