This repository has been archived by the owner on Jul 9, 2021. It is now read-only.
Organize async task queues by network #375
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
BMillman19
requested review from
abandeali1,
dekz,
fabioberger and
LogvinovLeon
as code owners
fabioberger
suggested changes
Feb 7, 2018
| private _start() { | ||
| this._queueIntervalIdIfExists = intervalUtils.setAsyncExcludingInterval( | ||
| async () => { | ||
| const task = this._queue.shift(); |
There was a problem hiding this comment.
Rename task to taskAsync
| await task(); | ||
| }, | ||
| this._queueIntervalMs, | ||
| _.noop, |
There was a problem hiding this comment.
We should actually handle the error here. If we encounter one, let's log the error to Rollbar and stop the thrown error from crashing the process.
There was a problem hiding this comment.
Ah, I see this is done in the individual actions. It would be cleaner to move it up here, then it only lives in one place.
There was a problem hiding this comment.
Hmm its a bit tricky because the error reporting is asynchronous, let's sync up if theres a way to fix this
| private _dispenseAsset(req: express.Request, res: express.Response, requestedAssetType: RequestedAssetType) { | ||
| const networkId = req.params.networkId; | ||
| const recipient = req.params.recipient; | ||
| const networkConfig = _.get(this._networkConfigByNetworkId, networkId); |
There was a problem hiding this comment.
Let's simply index in here rather then use _.get.
* development: (24 commits) Fix Remco's github name in CODEOWNERS Fix ABI error message Stop using definite assignment assertion cause prettier doesn't handle that Special-case ZRXToken snake case conversion Fix linter errors Generate contract wrappers on pre-build Add missing async Remove noImplicitThis Tslint disable no-consecutive-blank-lines in generated files Change compiled sources in contracts Change utils Change tests Add base_contract.ts Remove generated files .gitignore gemerated files Change the list of generated wrappers Change contract templates Add indices for index parameters so that their names don't collide Use abi-gen for events in 0x.js Fix artifacts path ...
dekz
reviewed
Feb 7, 2018
| dispenserTask = dispenseAssetTasks.dispenseEtherTask(recipient, networkConfig.web3); | ||
| break; | ||
| case RequestedAssetType.WETH: | ||
| dispenserTask = dispenseAssetTasks.dispenseTokenTask(recipient, requestedAssetType, networkConfig.zeroEx); |
There was a problem hiding this comment.
Can these be defaulted to the same case statement body?
case RequestedAssetType.WETH:
case RequestedAssetType.ZRX:
dispenserTask = dispenseAssetTasks.dispenseTokenTask(recipient, requestedAssetType, networkConfig.zeroEx);
break;
fabioberger
approved these changes
Feb 7, 2018
This was referenced May 10, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
Generalized the
RequestQueueinto aDispatchQueuethat serializes any arbitrary asynchronous tasks.Motivation and Context
Because the same address was being used to dispense both ETH and ZRX, sometimes the transaction nonces would get messed up. This fixes that problem by serializing transactions on a per network basis and only advancing the queue once the in-flight transaction resolves.
How Has This Been Tested?
Local testing
Types of changes
Checklist: