Skip to content
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

zeroex: remove checksum from address fields in Order when marshalling to JSON #262

Merged
merged 2 commits into from Jul 16, 2019

Conversation

hrharder
Copy link

Overview

Simply adding a call to strings.ToLower() on all address fields when marshalling a SignedOrder to JSON. Tests (run with make test-all) pass and functionality seems normal. Please advise if a different approach is preferred.

Should close #257.

Notes

Here is the output I got from a mesh_subscription message after adding a valid order (compare to sample in original issue).

{
	"jsonrpc": "2.0",
	"method": "mesh_subscription",
	"params": {
		"subscription": "0x5375e6c2d24cb11874a79ea90e7851f",
		"result": [{
			"fillableTakerAssetAmount": "0",
			"kind": "EXPIRED",
			"orderHash": "0x222f59940699cb7f7743d68e95cc952eabef58dd3ef22629811f0da6ff8b29aa",
			"signedOrder": {
				"makerAddress": "0x5409ed021d9299bf6814279a6a1411a7e866a631",
				"makerAssetData": "0xf47261b000000000000000000000000034d402f14d58e001d8efbe6585051bf9706aa064",
				"makerAssetAmount": "1000000000000000000",
				"makerFee": "0",
				"takerAddress": "0x0000000000000000000000000000000000000000",
				"takerAssetData": "0xf47261b000000000000000000000000025b8fe1de9daf8ba351890744ff28cf7dfa8f5e3",
				"takerAssetAmount": "98000000000000000000",
				"takerFee": "0",
				"senderAddress": "0x0000000000000000000000000000000000000000",
				"exchangeAddress": "0x48bacb9266a570d521063ef5dd96e61686dbe788",
				"feeRecipientAddress": "0x0000000000000000000000000000000000000000",
				"expirationTimeSeconds": "1563239813",
				"salt": "50548472960658633474509830344688430210810445656575157876862263167070060655407",
				"signature": "0x1c197ae36570bec706392e00594be1127caafe36f4a8093ed558039056cbb709553101ac01526e768d1d6c575cc7bf65dca4e140c27cb52228f2fba6278e19426a02"
			},
			"txHashes": []
		}]
	}
}

Let me know if think this should be done somewhere else, differently, etc... Or if someone else has already tackled it!

zeroex/order.go Outdated
MakerAssetData: makerAssetData,
MakerAssetAmount: s.MakerAssetAmount.String(),
MakerAssetAmount: strings.ToLower(s.MakerAssetAmount.String()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hrharder the MakerAssetAmount should be a numerical number and therefore does not need to be lowercased.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes -- thank you 🤦‍♂

zeroex/order.go Outdated
@@ -351,17 +352,17 @@ func (s SignedOrder) MarshalJSON() ([]byte, error) {
}

signedOrderBytes, err := json.Marshal(SignedOrderJSON{
MakerAddress: s.MakerAddress.Hex(),
MakerAddress: strings.ToLower(s.MakerAddress.String()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MakerAddress: strings.ToLower(s.MakerAddress.String()),
MakerAddress: strings.ToLower(s.MakerAddress.Hex()),

Nit: String() simply points to Hex() under-the-hood and exists as an implementation of the stringer interface. I feel it's slightly more expressive to use Hex() here. Mind making this change in the other places too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, I totally agree. I tried .String() (before checking implementation) to see if go-ethereum offered a native non-checksummed stringer, and never reverted.

and remove un-necessary strings.Lower call on number value
@fabioberger fabioberger merged commit cd1dc69 into 0xProject:development Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mesh: addresses are returned as checksummed
2 participants