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

feat: OrderExample #4

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

2hyunbin
Copy link

@traderben
Reflected your feedback. I made an example of placing an order using the existing sign method as it is.

Copy link
Contributor

@traderben traderben left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together! Just a few comments that hopefully shouldn't take too long

examples/OrderExample.tsx Outdated Show resolved Hide resolved
examples/OrderExample.tsx Outdated Show resolved Hide resolved
examples/OrderExample.tsx Outdated Show resolved Hide resolved
examples/OrderExample.tsx Outdated Show resolved Hide resolved
examples/OrderExample.tsx Show resolved Hide resolved
examples/OrderExample.tsx Show resolved Hide resolved
examples/OrderExample.tsx Outdated Show resolved Hide resolved
examples/OrderExample.tsx Outdated Show resolved Hide resolved
examples/OrderExample.tsx Outdated Show resolved Hide resolved
examples/OrderExample.tsx Outdated Show resolved Hide resolved
@2hyunbin
Copy link
Author

@traderben
Thank you for your detailed feedback. I have modified the code according to the requirements.

Copy link
Contributor

@traderben traderben left a comment

Choose a reason for hiding this comment

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

Thanks for updating. Just a couple more issues

const orderWire = orderRequestToOrderWire(orderRequest);
const orderAction = orderWiresToOrderAction([orderWire]);

if (!PRIVATE_KEY || PRIVATE_KEY === "0x-your-secret-key") {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit move this to above where you initialize wallet otherwise the example will crash there

throw new Error("Invalid order type");
}

const orderRequest: OrderRequest = {
Copy link
Contributor

Choose a reason for hiding this comment

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

move this into orderExample. asset should be 0 and remove assetId field. Having two fields for the same thing can lead to inconsistency and confusion

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.

2 participants