-
Notifications
You must be signed in to change notification settings - Fork 153
[TRIVIAL] Migrate zeroex to use alloy bytes types #3931
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
Conversation
jmg-duarte
left a comment
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.
LGTM overall but some notes
| IZeroex::LibNativeOrder::LimitOrder { | ||
| makerToken: zeroex_order.order().maker_token.into_alloy(), | ||
| takerToken: zeroex_order.order().taker_token.into_alloy(), | ||
| makerToken: zeroex_order.order().maker_token, | ||
| takerToken: zeroex_order.order().taker_token, | ||
| makerAmount: zeroex_order.order().maker_amount, | ||
| takerAmount: zeroex_order.order().taker_amount, | ||
| takerTokenFeeAmount: zeroex_order.order().taker_token_fee_amount, | ||
| maker: zeroex_order.order().maker.into_alloy(), | ||
| taker: zeroex_order.order().taker.into_alloy(), | ||
| sender: zeroex_order.order().sender.into_alloy(), | ||
| feeRecipient: zeroex_order.order().fee_recipient.into_alloy(), | ||
| pool: zeroex_order.order().pool.into_alloy(), | ||
| maker: zeroex_order.order().maker, | ||
| taker: zeroex_order.order().taker, | ||
| sender: zeroex_order.order().sender, | ||
| feeRecipient: zeroex_order.order().fee_recipient, | ||
| pool: zeroex_order.order().pool, | ||
| expiry: zeroex_order.order().expiry, | ||
| salt: zeroex_order.order().salt.into_alloy(), | ||
| }, | ||
| IZeroex::LibSignature::Signature { | ||
| signatureType: zeroex_order.order().signature.signature_type, | ||
| v: zeroex_order.order().signature.v, | ||
| r: zeroex_order.order().signature.r.into_alloy(), | ||
| s: zeroex_order.order().signature.s.into_alloy(), | ||
| r: zeroex_order.order().signature.r, | ||
| s: zeroex_order.order().signature.s, | ||
| }, | ||
| zeroex_order.order().taker_amount, |
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.
With all these .order() I wonder if we couldn't just make a function that returns a tuple with these 3 args and hides the .order() calls under a single one inside
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 ended up going with let order = zeroex_order.order() and referenced that variable instead.
Description
Migrates zereox related logic to use alloy's bytes types (Address, B256). I didn't touch the
U256types yet since they tend to be more complicate.How to test
existing e2e tests