-
Notifications
You must be signed in to change notification settings - Fork 461
Fix remaining no tx assigns #715
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
Fix remaining no tx assigns #715
Conversation
Just wonder is it all of them? Like do we really using |
ser, this is work progress [WIP] Just created a PR to run the actions cause I'm lazy. 😄 |
TL;DR - this PR is fine to merge but in some future PRs, we must remove these @janndriessen What concerns me about this commit (a20b1c4) is that the Here is a pseudocode example: let someErc = SomeERC.init(...)
someErc.transaction.callOnBlock = .exact(555_111)
/// This will probably fail due to `callOnBlock` but also trigger setting
/// `someErc.transaction` to `someErc.contract.transaction`
try? await someErc.executeWriteTransaction()
/// Expected to be called on the latest block but instead uses block number 555_111
try? await someErc.readSomeData() These types of bugs are the primary reason why we should remove all |
For sure you don't have to convince me that these classes have bad designs. Zooming out, I wouldn't even concentrate on adding/maintaining such classes - for me this is convenience/ nice to have. If the library is built modular enough, an ERC class like this should be easily buildable by anyone themselves. |
Also, all this kind of things should go on a list for future design choices for 4.x.x or other future versions. |
Summary of Changes
Fixes #712
WriteOperation.transaction
ignoresERC*.transaction
settings #712By submitting this pull request, you are confirming the following:
develop
branch.