add SetSubnetOwner to Chain interface
#2031
Conversation
dhrubabasu
requested review from
abi87,
danlaine and
StephenButtolph
as code owners
vms/platformvm/state/state.go
Outdated
| type wrappedFxOwner struct { | ||
| owner fx.Owner | ||
| size int | ||
| } | ||
|
|
||
| func wrappedFxOwnerSize(_ ids.ID, w *wrappedFxOwner) int { | ||
| if w == nil { | ||
| return ids.IDLen + constants.PointerOverhead | ||
| } | ||
| return ids.IDLen + w.size + constants.PointerOverhead | ||
| } |
There was a problem hiding this comment.
I understand this wrapper is just to be able to use fx.Owner in a sizedCache without recalculating the size every time.
The wrapper feels like a band-aid to me. I'd add a size method to fx.Owner interface to let ever owner be used in a sizedCache. Also fx.Owner implementation may be able to calculate statically their serialized size, without need to actually serialize data
There was a problem hiding this comment.
I didn't think it made sense to have a Bytes() like method for fx.Owner since the normal usage is passing it into another struct. This is a special case since we're explicitly tracking the owners separately so I'd like to keep it contained here
There was a problem hiding this comment.
I would add a Size() method to fx.Owner, not a Bytes() one.
I don't see why this is a special case. I can imagine other clients, possibly in other VMs may be interested in caching fx.Owners. So by having Size() method defined we'd avoid them the wrapper thing.
Also I'd like to avoid the wrapper in the P-chain. state is already pretty complex, I really don't see why we should add ad-hoc machinery to handle objects serialization and caching. We should push all of those machinery to the object themselves and slim down P-chain state.
There was a problem hiding this comment.
I would prefer implementing this wrapper as it's currently done for now and if we find that we keep needing this in other places we can consider implementing it on the fx package. I like package being able to define their own abstractions and keeping the interface as minimal as possible
This will not require a db migration, I'm lazily populating the subnetOwner db. |
vms/platformvm/state/state.go
Outdated
| // Subnet ID --> Owner of the subnet | ||
| subnetOwners map[ids.ID]fx.Owner | ||
| subnetOwnerCache cache.Cacher[ids.ID, *wrappedFxOwner] // cache of subnetID -> owner if the entry is nil, it is not in the database |
There was a problem hiding this comment.
feel free to ignore this comment: It's consistent w/ the rest of the code but I personally don't think we need comments here.
There was a problem hiding this comment.
I agree, we'll keep it for now but I'll probably make a follow-up cleaning up the comments
vms/platformvm/state/state.go
Outdated
| type wrappedFxOwner struct { | ||
| owner fx.Owner | ||
| size int | ||
| } | ||
|
|
||
| func wrappedFxOwnerSize(_ ids.ID, w *wrappedFxOwner) int { | ||
| if w == nil { | ||
| return ids.IDLen + constants.PointerOverhead | ||
| } | ||
| return ids.IDLen + w.size + constants.PointerOverhead | ||
| } |
There was a problem hiding this comment.
I would prefer implementing this wrapper as it's currently done for now and if we find that we keep needing this in other places we can consider implementing it on the fx package. I like package being able to define their own abstractions and keeping the interface as minimal as possible
| subnetID := subnetID | ||
| owner := owner |
There was a problem hiding this comment.
I thought loop variables in go were created once and the value was updated with each iteration, so I thought we wouldn't need these copies here. I think it's safe to remove these lines but i could be wrong.
There was a problem hiding this comment.
You are correct, it is safe to remove but I'd like to keep them since it's easier to understand that this is correct
There was a problem hiding this comment.
I think the linter is going to yell at us because we are passing references to these values into functions. While it is true that those functions don't hold a reference to them... it feels like this is more clearly correct (until go 1.22)
| @@ -834,9 +893,14 @@ func (s *state) GetSubnetOwner(subnetID ids.ID) (fx.Owner, error) { | |||
| return nil, fmt.Errorf("%q %w", subnetID, errIsNotSubnet) | |||
| } | |||
|
|
|||
| s.SetSubnetOwner(subnetID, subnet.Owner) | |||
There was a problem hiding this comment.
This feels a bit weird (to be performing a write on the read)... But I think it's ok
| subnetID := subnetID | ||
| owner := owner |
There was a problem hiding this comment.
I think the linter is going to yell at us because we are passing references to these values into functions. While it is true that those functions don't hold a reference to them... it feels like this is more clearly correct (until go 1.22)
Generally, the |
No description provided.