Skip to content
This repository has been archived by the owner on Oct 11, 2024. It is now read-only.

Remove OrderWatcher in-memory state #905

Open
albrow opened this issue Aug 11, 2020 · 0 comments
Open

Remove OrderWatcher in-memory state #905

albrow opened this issue Aug 11, 2020 · 0 comments
Labels
tech debt A code quality issue is not urgent but could cause problems down the line if not addressed

Comments

@albrow
Copy link
Contributor

albrow commented Aug 11, 2020

The switch from LevelDB to using SQL and Dexie.js makes it possible to remove all in-memory state from orderwatch.Watcher. There are currently still some pieces of in-memory state remaining.

addAssetDataAddressToEventDecoder adds an entry to knownERC20Addresses, knownERC721Addresses, etc. inside decoder.Decoder. This was designed to tackle a specific problem: we need to know the contract ABI (depends on the type of the token) in order to properly decode contract events, but some events have the same signature and cannot be easily differentiated. To solve this problem, we keep track of every unique asset data that we see and store the token type (based on the asset data prefix) in a map. A different way to work around this problem is to sequentially try to decode the event for each token type (e.g. ERC20, ERC721, ERC1155), until we find one that decodes without an error. This would allow us to eliminate some complicated and error-prone code that maintains the map of asset data to token type.

Currently, Decoder.Decode looks like this:

// Decode attempts to decode the supplied log given the event types relevant to 0x orders. The
// decoded result is stored in the value pointed to by supplied `decodedLog` struct.
func (d *Decoder) Decode(log types.Log, decodedLog interface{}) error {
	if isKnown := d.isKnownERC20(log.Address); isKnown {
		return d.decodeERC20(log, decodedLog)
	}
	if isKnown := d.isKnownERC721(log.Address); isKnown {
		return d.decodeERC721(log, decodedLog)
	}
	if isKnown := d.isKnownERC1155(log.Address); isKnown {
		return d.decodeERC1155(log, decodedLog)
	}
	if isKnown := d.isKnownExchange(log.Address); isKnown {
		return d.decodeExchange(log, decodedLog)
	}

	return UntrackedTokenError{Topic: log.Topics[0], TokenAddress: log.Address}
}

It can be re-written to something like this:

func (d *Decoder) Decode(log types.Log, decodedLog interface{}) error {
	// First try decoding as an ERC20 event
	if err := d.decodeERC20(log, decodedLog); err != nil {
		// Depending on the type of error, we may need to return early here.
        } else {
		// Decoding was successful, which means this is in fact an ERC20 event.
		return nil
	}

	// If the above didn't work, try to decode as an ERC721 event.
	if err := d.decodeERC721(log, decodedLog); err != nil {
		// Depending on the type of error, we may need to return early here.
        } else {
		// Decoding was successful, which means this is in fact an event for an ERC721 token.
		return nil
	}
	
	// Keep going for ERC721 and any other supported token types...

	// If none of the above worked, return an error. This error message might need to be tweaked or improved
	return errors.New("Unknown event type")
}

knownERC20Addresses, knownERC721Addresses, etc. are also used in handleBlockEvents to find orders in the database that were potentially affected by a given event. Specifically, the logic takes place in Decoder.FindEventType. Instead of using a map of asset data to token type here, we can leverage the new database features to simply query for each possible token type/asset data prefix. To do this efficiently, we may need to combine one or more query. So for example if we cannot determine the token type based on the event itself, but we know the token address, we first look for orders with the asset data erc20AssetDataPrefix + tokenAddress, then we look for orders with the asset data erc721AssetDataPrefix + tokenAddress, and so on.

@albrow albrow added the tech debt A code quality issue is not urgent but could cause problems down the line if not addressed label Aug 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tech debt A code quality issue is not urgent but could cause problems down the line if not addressed
Projects
None yet
Development

No branches or pull requests

1 participant