Mock the reward functionality of the native-manager #646
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
richardpringle
requested review from
iansuvak,
geoff-vball,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
richardpringle
force-pushed
the
bad-expectations
branch
from
9f7de88 to
4ed2f6d
Compare
richardpringle
commented
Nov 14, 2024
Comment on lines
-258
to
-263
| vm.mockCall({ | ||
| callee: account, | ||
| msgValue: amount, | ||
| data: "", // implies receive() | ||
| returnData: "" | ||
| }); |
There was a problem hiding this comment.
This wasn't doing anything
iansuvak
reviewed
Nov 15, 2024
There was a problem hiding this comment.
I'm not hard opposed to this and agree with your assessment that the current implementation has problems, but not sure that I love divergence (albeit tiny) from the actual contract we want to test and the contract we are actually testing. _reward is simple enough though that this is probably fine here.
| } | ||
|
|
||
| function _setUp() internal override returns (IValidatorManager) { | ||
| // Construct the object under test | ||
| app = new NativeTokenStakingManager(ICMInitializable.Allowed); | ||
| app = new MockNativeTokenStakingManager(ICMInitializable.Allowed); |
There was a problem hiding this comment.
I'm not sure that MockNativeTokenStakingManager is the best name here since you aren't really mocking here but monkeypatching instead. How about TestableNativeTokenStakingManager
richardpringle
force-pushed
the
bad-expectations
branch
from
4ed2f6d to
d2aa93d
Compare
iansuvak
approved these changes
Nov 15, 2024
geoff-vball
approved these changes
Nov 18, 2024
bernard-avalabs
approved these changes
Nov 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why this should be merged
We were actually triggering logic in a function named
_expect*. It was super misleading and really hard to track down why that was happening. Instead, I've properly mocked the functionality, triggering thevm.deallogic when the_rewardfunction is called instead of afterwards.How this works
I've effectively monkey-patched the
_rewardFunction.How this was tested
It is a test 😉
How is this docuomented
There's a code comment that was already there that I moved.