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.

Sign up for GitHub

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

Jump to bottom

Interface icx_createorder RPC #398

Merged
merged 6 commits into from
Jun 28, 2021
Merged

Interface icx_createorder RPC #398

merged 6 commits into from
Jun 28, 2021

Conversation

surangap
Copy link
Contributor

What kind of PR is this?:

/kind feature

What this PR does / why we need it:

This PR will interface icx_createorder RPC to jellyfish

Which issue(s) does this PR fixes?:

Fixes #

Additional comments?:

Refer following resources to learn how ICX works
https://github.com/DeFiCh/pinkpaper/tree/main/interchain-exchange
https://github.com/DeFiCh/pinkpaper/blob/main/interchain-exchange/fees.md
https://github.com/DeFiCh/ain/wiki/Interchain-Exchange-Tutorial

@defichain-bot defichain-bot added the kind/feature New feature request label Jun 21, 2021
@codeclimate
Copy link

codeclimate bot commented Jun 21, 2021 •
edited
Loading

Code Climate has analyzed commit 0a50bfc and detected 0 issues on this pull request.

View more on Code Climate.

@netlify
Copy link

netlify bot commented Jun 21, 2021 •
edited
Loading

✔️ Deploy Preview for jellyfish-defi ready!

🔨 Explore the source changes: 0a50bfc

🔍 Inspect the deploy log: https://app.netlify.com/sites/jellyfish-defi/deploys/60d94ff57c2f5600079c12f0

😎 Browse the preview: https://deploy-preview-398--jellyfish-defi.netlify.app

@github-actions
Copy link

github-actions bot commented Jun 21, 2021 •
edited
Loading

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/jellyfish/dist/index.umd.js 21.03 KB (+0.98% 🔺) 421 ms (+0.98% 🔺) 194 ms (+4.44% 🔺) 614 ms

@codecov
Copy link

codecov bot commented Jun 21, 2021 •
edited
Loading

Codecov Report

Merging #398 (0a50bfc) into main (58fba5e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #398      +/-   ##
==========================================
+ Coverage   97.30%   97.31%   +0.01%     
==========================================
  Files          95       96       +1     
  Lines        2778     2793      +15     
  Branches      351      354       +3     
==========================================
+ Hits         2703     2718      +15     
  Misses         75       75
Impacted Files Coverage Δ
...es/jellyfish-api-core/src/category/icxorderbook.ts 100.00% <100.00%> (ø)
packages/jellyfish-api-core/src/index.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58fba5e...0a50bfc. Read the comment docs.

fuxingloh
fuxingloh reviewed Jun 21, 2021
View reviewed changes
Copy link
Contributor

@fuxingloh fuxingloh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

initial code style comments, haven't checked it detaily yet

packages/jellyfish-api-core/src/category/icxorderbook.ts Outdated Show resolved Hide resolved
website/docs/jellyfish/api/icxorderbook.md Outdated Show resolved Hide resolved
packages/jellyfish-api-core/src/category/icxorderbook.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
@surangap surangap force-pushed the surangap/icx-createorder branch from 1927421 to c4349f8 Compare June 21, 2021 16:30
@surangap surangap mentioned this pull request Jun 21, 2021
Merged
@surangap surangap marked this pull request as ready for review June 22, 2021 06:23
@surangap surangap requested a review from fuxingloh June 22, 2021 06:23
fuxingloh
fuxingloh suggested changes Jun 22, 2021
View reviewed changes
Copy link
Contributor

@fuxingloh fuxingloh left a comment

Choose a reason for hiding this comment

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

A few JS testing style notes

packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/common.test.ts Outdated Show resolved Hide resolved
@surangap surangap requested a review from fuxingloh June 23, 2021 09:25
fuxingloh
fuxingloh suggested changes Jun 23, 2021
View reviewed changes
Copy link
Contributor

@fuxingloh fuxingloh left a comment

Choose a reason for hiding this comment

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

should specify the chain in your it('') for better clarify, making it easier to read

anyway love the code and test consistency!

packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
@surangap surangap force-pushed the surangap/icx-createorder branch from 4ab2c2c to 6214b30 Compare June 25, 2021 04:18
canonbrother
canonbrother requested changes Jun 25, 2021
View reviewed changes
Copy link
Contributor

@canonbrother canonbrother left a comment

Choose a reason for hiding this comment

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

overall good.. just minor

packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/__tests__/category/icxorderbook/createOrder.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-api-core/src/category/icxorderbook.ts Outdated Show resolved Hide resolved
@surangap surangap force-pushed the surangap/icx-createorder branch from 9f5d7c2 to 920aeec Compare June 25, 2021 05:54
@surangap surangap requested a review from canonbrother June 25, 2021 05:55
@fuxingloh fuxingloh enabled auto-merge (squash) June 28, 2021 04:30
@fuxingloh fuxingloh merged commit 15aef67 into main Jun 28, 2021
@fuxingloh fuxingloh deleted the surangap/icx-createorder branch June 28, 2021 04:47
@surangap surangap mentioned this pull request Jun 29, 2021
Closed
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@canonbrother canonbrother canonbrother approved these changes

@fuxingloh fuxingloh fuxingloh approved these changes

@ivan-zynesis ivan-zynesis Awaiting requested review from ivan-zynesis ivan-zynesis is a code owner

@jingyi2811 jingyi2811 Awaiting requested review from jingyi2811

@monstrobishi monstrobishi Awaiting requested review from monstrobishi

@thedoublejay thedoublejay Awaiting requested review from thedoublejay

Assignees

@surangap surangap

Labels
kind/feature New feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@surangap @fuxingloh @canonbrother @defichain-bot