-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
deprecated estimateGas and buildTransaction in v5 contract #2438 #2459
Conversation
@kclowes take a look at this PR. I closed my other PR because I accidently pulled from master and REALLY messed it up. |
I fixed the issue with the combomethod annotation. Also, @kclowes I took care of the warnings in the test in the asyncify-contract branch already so I was going to leave these. thoughts?? |
For deprecations, we prefer to change all of the instances of our code over to the new function name and then have one test to be sure the deprecation warning gets shown just so that we don't have a bunch of noise in our test output. For example something like:
|
@kclowes makes sense I will take a look at this. |
@kclowes this one should be good to go now. |
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.
Looks good to me! I left a few questions, but after those are addressed, should be good to go!
This should be good to go |
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.
Thank you! This looks good to me! I'm going to change the newsfragment from misc
to feature
just so it gets highlighted in the release notes. Any misc
s only show up as the PR number.
Deprecating buildTransaction and estimateGas in the v5 branch. I will remove them in the asyncify-contract branch so they are effectively removed in v6
Related to Issue #1416
Todo: