chore: add more validation to MsgRun & MsgAddPackage
#1646
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1646 +/- ##
==========================================
- Coverage 56.25% 56.11% -0.15%
==========================================
Files 425 439 +14
Lines 64408 66194 +1786
==========================================
+ Hits 36235 37147 +912
- Misses 25351 26155 +804
- Partials 2822 2892 +70
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
leohhhn
requested review from
jaekwon,
piux2 and
zivkovicmilos
as code owners
leohhhn
changed the title
MsgRun & MsgAddPackage
|
Please give me a minute to review this, I'm sensing it's a touchy PR 🙏 |
|
cc/ @jaekwon, from review meeting: Is there any reason why |
There was a problem hiding this comment.
Why would you move the Package validation outside the keeper methods, that validate it upon execution, and into ValidateBasic, which should be a quick sanity check for validity?
We use Package.Validate and ValidateBasic in the same sense how we use CheckTx and DeliverTx - one is a quick sanity check, and the other is detailed validation (VM processing).
We execute ValidateBasic for top-level validations (ex. when the node receives a tx message from gossip), and do additional validations (execution validations) when the node actually commits it to the VM.
I am not convinced this change is valid
| @@ -50,12 +50,19 @@ var ( | |||
| // file names must contain dots. | |||
| // NOTE: this is to prevent conflicts with nested paths. | |||
| func (mempkg *MemPackage) Validate() error { | |||
| if mempkg == nil { | |||
| return errors.New(fmt.Sprintf("package is nil")) | |||
There was a problem hiding this comment.
You should format errors with fmt.Errorf.
Looking at this again, you don't even format an error here, this entire call is useless - define it as a package-level error
| if !rePkgName.MatchString(mempkg.Name) { | ||
| return errors.New(fmt.Sprintf("invalid package name %q, failed to match %q", mempkg.Name, rePkgName)) | ||
| } | ||
| if !rePkgOrRlmPath.MatchString(mempkg.Path) { | ||
| return errors.New(fmt.Sprintf("invalid package/realm path %q, failed to match %q", mempkg.Path, rePkgOrRlmPath)) | ||
| } | ||
| if mempkg.IsEmpty() { | ||
| return errors.New(fmt.Sprintf("package %q contains no files", mempkg.Name)) |
There was a problem hiding this comment.
You should format errors with fmt.Errorf
| @@ -50,12 +50,19 @@ var ( | |||
| // file names must contain dots. | |||
| // NOTE: this is to prevent conflicts with nested paths. | |||
| func (mempkg *MemPackage) Validate() error { | |||
There was a problem hiding this comment.
Please add unit tests for MemPackage validation
| if err := msg.Package.Validate(); err != nil { | ||
| return ErrInvalidPackage(err.Error()) | ||
| } |
| if err := msg.Package.Validate(); err != nil { | ||
| return ErrInvalidPackage(err.Error()) | ||
| } |
| ) | ||
|
|
||
| func (e InvalidPkgPathError) Error() string { return "invalid package path" } | ||
| func (e InvalidPackageError) Error() string { return "package validation failed" } |
There was a problem hiding this comment.
Please add a documentation reference in this PR as to what is a "valid package" 🙏
|
This has been hanging mainly because I assume I will spend too much time due to my lack of knowledge on how the keeper works - I will give it a shot this week and try to incorporate the change you requested. Will ping you when I'm done 👍 EDIT: I asked for your opinion on this privately but received no reply. I'm focusing on other priorities so I will not work on this now. |
Description
This PR adds more validation to
MsgRun&MsgAddPkg. It does this by adding a few checks toMemPackage'sValidate(), and then incorporating checks onMemPackagewithinMsgRun&MsgAddPkg.Contributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description