-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: avoid Instantiation of banker on pure packages #2248
Conversation
a981492
to
4a746b3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2248 +/- ##
==========================================
- Coverage 63.40% 63.40% -0.01%
==========================================
Files 565 565
Lines 79454 79461 +7
==========================================
+ Hits 50375 50379 +4
- Misses 25688 25690 +2
- Partials 3391 3392 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work in a real life scenario, as CurrentRealm()
skips frames of pure packages. (If you want a good side-task, change NewCodeRealm
to work only with PkgPath's that match the rules of gnolang.IsRealmPath
)
I suggest:
- Having a function like
assertCallerIsRealm
. - This should get the frame of the function where this is called (I think m.Frames[m.NumFrames()-2]).
- Then call
LastPackage
and make sure its PkgPath matches that of gnolang.IsRealmPath.
Thanks!
4fc990d
to
e7ade2c
Compare
Hello thanks a lot for your review and for the great guidance :)
frame := m.Frames[m.NumFrames()-2]
if !frame.LastPackage.IsRealm() {
m.Panic(typedString("caller is not a realm"))
}
|
6e9ceab
to
7633139
Compare
746e3ba
to
fb3e1f8
Compare
fb3e1f8
to
a4f16fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, generally looks good.
To justify why len(m.Frames)-2
, I suggest you dump the machine in the point you use it (in the native function), just for your own reference. len(frames)-1 will be the frame of assertCallerIsRealm
, len(frames)-2 is the frame of its caller, ie. GetBanker
. We can then use LastPackage
to determine which package called it.
45e13f3
to
97fc314
Compare
97fc314
to
d5639e5
Compare
Apologies, revert the changes in tests/files/extern and remove that dir from the fmt rule.
|
No worries :) |
Related to #2192
Don't really know if this is the right approach. If you have another idea of how to implement this security check please let me know :)
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description