Skip to content

refactor: use reflect.TypeFor#682

Open
box4wangjing wants to merge 2 commits into
singnet:masterfrom
box4wangjing:master
Open

refactor: use reflect.TypeFor#682
box4wangjing wants to merge 2 commits into
singnet:masterfrom
box4wangjing:master

Conversation

@box4wangjing
Copy link
Copy Markdown

Try to use better api reflect.TypeFor instead of reflect.TypeOf when we have known the type.
More info golang/go#60088

Inspired by #653 and replace all.

Signed-off-by: box4wangjing <box4wangjing@outlook.com>
@box4wangjing
Copy link
Copy Markdown
Author

@semyon-dev Hi, Could you please review this PR at your convenience? Thank you very much.

@semyon-dev semyon-dev self-requested a review May 22, 2026 10:07
Copy link
Copy Markdown
Member

@semyon-dev semyon-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migration itself looks safe and preserves the existing value-type semantics (T instead of *T).

One thing worth double-checking is that TypedAtomicStorageImpl intentionally expects non-pointer reflected types, while runtime usage operates on pointers (*ModelKey, *ModelData, etc.). This behavior already existed before the refactor, so the PR does not introduce it, but it's an important invariant for future changes.

@semyon-dev
Copy link
Copy Markdown
Member

One small note: the PR description says "replace all", but there are still several handwritten storage-related usages of reflect.TypeOf(T{}) remaining (for example in payment_channel_storage.go, prepaid_storage.go, and license_storage.go).

The remaining .pb.go generated files are probably fine to leave untouched, but it may be worth either:

  • updating the PR description to clarify the scope, or
  • including the remaining storage-layer replacements for consistency.

@semyon-dev semyon-dev added the help wanted Extra attention is needed label May 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

help wanted Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants