Tech Debt PR Including Release Version Bump #42
Merged
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
added 5 commits
December 22, 2022 17:37
…ate, remove the deprecated dry run mixins
…d... but still need the multidispatcher
tzaffi
commented
Dec 24, 2022
tzaffi
commented
Dec 24, 2022
| @@ -234,62 +234,6 @@ def get_return(f, g): | |||
|
|
|||
| raise ValueError("Unhanlded PredicateKind {kind} for predicate {predicate}") | |||
|
|
|||
| @classmethod | |||
| def inputs_and_invariants( | |||
There was a problem hiding this comment.
removing this confusing/error-prone method
6 tasks
* small typing fix + EncodingType * type ignores
| roundtrip_teal, | ||
| args, | ||
| abi_method_signature=method_sig, | ||
| omit_method_selector=True, |
There was a problem hiding this comment.
@tzaffi Optional - Unless I missed it, I think some combinations of omit_method_selector + validation are untested. Ideally, all paths are exercised.
- I attached a git patch suggesting a minor change to to parameterize
validation:
diff.txt. Accounting foromit_method_selectorrequires more thought or possibly designing a different test. - If you prefer as is, then feel welcomed to resolve.
There was a problem hiding this comment.
I will not act on this suggestion but keep it open and link to it in #44. This is because sequencing the diff in this case does feel more complicated given all the upcoming changes.
There was a problem hiding this comment.
I've added a TODO section in #44 :
There was a problem hiding this comment.
Unresolving for easier spotting. Also, this is slated to be addressed in #45
michaeldiamant
approved these changes
Jan 3, 2023
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.
Removing:
Deprecated Mixins Originally Inherited from
py-algorand-sdkWith cost no longer coming back, and the soon-coming need to support simulate, remove the deprecated dry run mixins. It's not worth continuing to support now that we get this error:
Orphaned methods and functions removed in
graviton/dryrun.py:_fail()assert_pass()assert_reject()assert_status()assert_global_state_contains()assert_local_state_contains()DryRunHelper.guess()DryRunHelper.format_stack()DryRunHelper.find_delta_value()DryRunHelper.save_dryrun_request()Invariant.inputs_and_invariants()This overly-complex method was guilty of causing a test to provide false confidence with never being run for 8 months. It already was slated for removal with a
TODO:. So this is a good opportunity to remove.Refactoring Dry Run Execution Methods
Removing parameters
abi_argument_typesabi_return_typeAdding parameters
abi_method_signature: stromit_method_selector: boolvalidation: boolTODO
#40 was NOT tackled here. However, the refactoring work here paves the way to address it and #44 is a child PR which addresses.
Shall Refactor Dry Run Execution #44 be merged into this PR?