-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
529cac6
to
7e341a2
Compare
7e341a2
to
2da8b98
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.
Mostly looks good.
I see in the test cases that there are a number of edge cases around how proposal execution behaves. Are these listed out and documented somewhere?
Also I'm having trouble understanding where all the testdata
stuff is used in this PR.
"not authz by group account": { | ||
srcAccount: []byte("my-group-acct-addrss"), | ||
srcMsgs: []sdk.Msg{MyMsg{[]sdk.AccAddress{[]byte("any--other---address")}}}, | ||
srcHandler: alwaysPanicHandler(), |
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.
shouldn't this be mockHandler
if we actually want to test this case? (panic might get caught by the router)
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.
In this scenario the authentication decorator is tested. The handler should not be called as the payload message "signer" is not the groupAccount. This prevents access to random accounts.
The alwaysPanicHandler
makes sense as it would raise a panic and not an error. The panic would not be handled by the router.
As with the other PRs there is no "proper documentation" written. I have tried to define test cases for all the scenarios I could imagine so that they are a specification as code. Probably not what you were looking for. The |
@clevinson can you comment on documentation here? |
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.
Approving this, but would be great if we can track all the edge cases covered here as a documentation TODO. @clevinson maybe you can flag this in an issue and link to the test cases covered here?
I have created an issue and linked to the comment |
After #66
Resolves #38