-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Transaction confirmation workflow improvements #2466
Merged
Merged
Changes from 57 commits
Commits
Show all changes
91 commits
Select commit
Hold shift + click to select a range
5225a1a
transaction confirmation and signing logic moved to the Eth module, c…
e2d1b70
transaction confirmation and signing logic moved to the Eth module, c…
2407b9e
POC TransactionObserver
b8d4d72
parameters for the execute method updated
c456be2
Dependency handling of AbstractWeb3Module updated because of the remo…
717133c
last exposed factory from the web3 core removed. I've changed this be…
5a9bf13
AbstractObservedSendTransactionMethod renamed and updated for using t…
ff688d1
TransactionObserver POC
69b4d96
TransactionObserver updated
58169b3
TransactionObserver updated
4a38fb5
methods in Eth module updated
bc2f034
MethodFactory in eth-module updated
108270e
code style and other improvements in the eth-module
5746996
Dependecy handling updated
1ff399e
eslint executed and errors fixed
bfc7500
import updated in MethodFactory of the eth module
39b13f3
PromiEvent moved to core module
331b1ce
Merge branch '1.0' into enhacement/transaction-confirmations
c831d66
Merge branch '1.0' into enhacement/transaction-confirmations
53c442b
custom eth block methods logic moved to eth module for having a clear…
0de7249
custom eth method names improved
3efd3cc
methods checked and GetTransactionFromBlockMethod implemented in the …
ff0345c
exports updated in core-method module
e2efecc
TransactionObserver updated
43e7933
eslint executed and errors fixed
efc0798
build fixed
459d954
eth-contract dependencies fixed
2f44666
dependency handling fixed
518e542
TranasctionObserver with contract deployment tested and fixed
b0a2edd
MethodFactory in Eth module updated and TransactionObserver fixed
1d56911
TransactionObserver startSocketObserver method updated
53030b5
beforeExecution added to ObservedSendTransactionmethod
cd0f8a1
dependency handling checked and fixed
6824591
startSocketObserver updated for chain reorg in TransactionObserver
afc6920
unsused method removed, dependency removed and wrongly used method up…
863bb83
subscription and error handling of TransactionObserver improved
fc7c85c
error message updated
aab8b01
TransactionObserver improved
884fc8e
options handling of eth.Contract improved, dependency handling update…
dd9ed3c
naming fixed
f8f8a64
AbstractMethodFactoryTest updated, package.json depeendencies updated…
357800c
AbstractMethodTest updated
5464143
block methods tests updated in core-method module
597154c
transaction methods structure simplified
08bc754
imports updated in eth modules and parameters length validation move…
705ac24
promiEvent handling improved
d987b88
AbstractObservedTransactionMethod and the related test updated
103520e
AbstractGetTransactionFromBlockMethodTest created
01842e5
method tests updated in core-method module
3c64591
MethodProxyTest updated
f984315
TransactionReceiptValidator removed and logic moved to AbstractObserv…
dbcf3f5
TransactionObserver updated and AbstractObservedTransactionMethod imp…
e197457
AbstractMethodFactory, AbstractObservedTransactionMethod, EthSendTran…
f92c288
static getter with name 'Type' added for a better detection of the ty…
f249067
dependency handing updated in the eth-contract module
6c791bb
AbstractObservedTransactionMethodTest updated
09ae3fd
stop method removed because of the o'bserver.closed' property
b8f4d9c
TransactionObserver test created
6be4975
code style fixes and AbstractMethodFactoryTest updated. The core-meth…
619228a
contract deployment and calling of methods tested and fixed
f31c372
core module tests updated
3b7d739
Shh module tests upddated
09b50ea
EthSendTransactionMethodTest created in core-method module
e7f1b01
method tests updated in eth module
826d5e4
EthTest updated and MethodFactoryTest created
3513aba
eth module tests updated
3646b16
naming of the AbstractGetUncleMethod class in fixed
a01f856
eslint-plugin-jest updated and eslint executed
6d605cf
AbstractWeb3ModuleTest updated
3e6a89f
test case added to AbstractWeb3ModuleTest
e6907de
net module tests updated
56e642b
eth-accounts tests updated
f5bf42e
eth-personal module tests updated
1a0f8aa
code style improvements
3d8ae8e
MethodsProxyTest updated and athe accsessors for the arguments proper…
f4b4920
factory tests updated in eth-contract module
870716b
eth-contract methods tests updated
f39b189
code style improvements and AbiItemModelTest updated
46c9e62
AbstractMethodTest updated
87286dc
ens module tests updated
0d83389
Web3 class updated for the new ProviderDetector
ebba124
code style and jest config improved
1c30a5e
types updated
988e402
Merge branch '1.0' into enhacement/transaction-confirmations
346f159
code tested, eslintignore updated and package.json updated
cd608d4
dtslint dependency fixed
7409d6e
parsimon removed from root package.json and travis.yml updated
408a68d
duplicated env in travis.yml removed
1a77830
lerna bootstrap command updated in package.json with '--hoist'
54d3cd6
dtslint version in web3-providers package updated and 'npm ci' change…
ecef7a8
hoist removed from lerna bootstrap and dtslint downgraded
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Large diffs are not rendered by default.
Oops, something went wrong.
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
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
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
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
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
Oops, something went wrong.
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.
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.
Perhaps it's not my place to ask, but wouldn't this add a copy of the module to all methods? I haven't tested this branch, but instead of copying the whole module instance, perhaps modules could have a concrete
moduleInstance.config
? (I don't remember the full structure of the package, but this might also avoid circular references somewhere unexpected.)Maybe this is preemptive optimization, but I thought I'd put it out there.
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.
The garbage collector of JS will collect and remove the
AbstractMethod
after the execution of the method. TheAbstractMethod
classes will be created in theMethodProxy
when a declared method gets called. It will not copy themoduleInstance
each time. It will just set the reference of the pointer. This meansthis.moduleInstance
is just a "symlink" to the actual object.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.
I see. I wasn't sure if the
moduleInstance
would end up as a pointer or as a copy. It only takes one random line referencing the object to end up allocated till unmounting.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.
Maybe it works directly with the ES6 structure, but the
.umd.js
files don't seem to be correctly collected. Maybe for this release working on the.umd.js
transpiler settings would be beneficial. The image above is taken from the (semi-)production build of thegetBlock
example with webpack. https://codesandbox.io/s/0ykmkv2380There 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.
This is the actual production build heap snapshot after some digging. I've tested the
.umd.js
every now and then and its behavior strays quite a bit from its.esm.js
counterpart.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.
There are still some points I could optimize the memory usage and other stuff. But I will do this later because I've already broken the heap snapshot down from ~550mb to ~280mb. Thanks for your tests! :-)
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.
Sorry if I came across as pedantic. What worries me overall is that the
transpiling to browser JS might end up having unexpected behavior. Every
now and then it happens to me that the ES6 or CJS code works but the
browser code doesn't. Probably not a pressing issue, but something I'd
avoid neglecting since the package is widely used client-side, but the
tests don't strictly cover it.