-
Notifications
You must be signed in to change notification settings - Fork 414
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
Add and test JSON object matcher #819
Conversation
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.
@jackzampolin you really should cherry-pick in these commits to your branch.
This is great example of testing methods
(Or maybe we just merge these two commits to main?)
x/wasm/types/json_matching.go
Outdated
|
||
// isJSONObjectWithTopLevelKey checks if the given bytes are a valid JSON object | ||
// with exactly one top level key that is contained in the list of allowed keys. | ||
func isJSONObjectWithTopLevelKey(jsonBytes []byte, allowedKeys []string) error { |
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.
very nice function, doing what it should
x/wasm/types/json_matching.go
Outdated
} | ||
|
||
if !found { | ||
return sdkerrors.Wrapf(ErrTopKevelKeyNotAllowed, "JSON object has a top level key which is not allowed: '%s'", topLevelKey) |
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.
And return this error here. Removing the found
variable completely
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.
Right, done, thanks
@@ -72,6 +72,18 @@ var ( | |||
|
|||
// error if an address does not belong to a contract (just for registration) | |||
_ = sdkErrors.Register(DefaultCodespace, 22, "no such contract") | |||
|
|||
// ErrNotAJSONObject error if given data is not a JSON object | |||
ErrNotAJSONObject = sdkErrors.Register(DefaultCodespace, 23, "not a JSON object") |
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.
Good stuff with the unique error codes
}, | ||
"happy with excaped key": { | ||
src: []byte(`{"event\u2468thing": {"foo":"bar"}}`), | ||
allowedKeys: []string{"event⑨thing"}, |
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.
😄
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestIsJSONObjectWithTopLevelKey(t *testing.T) { |
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.
Wonderful test vectors here!
1924417
to
0e723a6
Compare
Rebased onto main as this is independent of the diff in #817. Feel free to merge into main or cherry-pick into the PR once @jackzampolin and maybe @alpe had a look. |
Minting fails now due to:
Please make that public ( |
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.
Happy to merge as soon as CI is happy (commented how to fix above)
0e723a6
to
1e892ac
Compare
Codecov Report
@@ Coverage Diff @@
## main #819 +/- ##
==========================================
+ Coverage 59.29% 59.34% +0.04%
==========================================
Files 50 51 +1
Lines 5884 5898 +14
==========================================
+ Hits 3489 3500 +11
- Misses 2143 2145 +2
- Partials 252 253 +1
|
JSON matching for #817