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

feat(jellyfish-transaction): Add updatemasternode dftx #1863

Merged
merged 34 commits into from
Dec 1, 2022
Merged

feat(jellyfish-transaction): Add updatemasternode dftx #1863

merged 34 commits into from
Dec 1, 2022

Conversation

marktanrj
Copy link
Contributor

@marktanrj marktanrj commented Nov 18, 2022 •
edited
Loading

What this PR does / why we need it:

  • updatemasternode DfTx parsing

Which issue(s) does this PR fixes?:

Fixes part of #1842

@github-actions github-actions bot added area/packages kind/feature New feature request labels Nov 18, 2022
@codeclimate
Copy link

codeclimate bot commented Nov 18, 2022 •
edited
Loading

Code Climate has analyzed commit d4d7736 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 3

View more on Code Climate.

@marktanrj marktanrj changed the title feat(jellyfish-api-core): Add updatemasternode dftx feat(jellyfish-transaction): Add updatemasternode dftx Nov 18, 2022
@netlify
Copy link

netlify bot commented Nov 18, 2022 •
edited
Loading

✅ Deploy Preview for jellyfishsdk ready!

Name Link
🔨 Latest commit d4d7736
🔍 Latest deploy log https://app.netlify.com/sites/jellyfishsdk/deploys/63884b2751b9c000081b8c70
😎 Deploy Preview https://deploy-preview-1863--jellyfishsdk.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@marktanrj marktanrj mentioned this pull request Nov 18, 2022
Closed
@codecov
Copy link

codecov bot commented Nov 19, 2022 •
edited
Loading

Codecov Report

Base: 84.52% // Head: 91.73% // Increases project coverage by +7.20% 🎉

Coverage data is based on head (d4d7736) compared to base (fd18179).
Patch coverage: 30.43% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1863      +/-   ##
==========================================
+ Coverage   84.52%   91.73%   +7.20%     
==========================================
  Files         358      361       +3     
  Lines       10414    10496      +82     
  Branches     1314     1321       +7     
==========================================
+ Hits         8802     9628     +826     
+ Misses       1537      825     -712     
+ Partials       75       43      -32
Impacted Files Coverage Δ
...ish-transaction/src/script/dftx/dftx_masternode.ts 67.56% <29.41%> (-32.44%) ⬇️
...ages/jellyfish-transaction/src/script/dftx/dftx.ts 95.68% <33.33%> (+4.44%) ⬆️
...ckages/jellyfish-transaction/src/script/mapping.ts 94.19% <33.33%> (+14.45%) ⬆️
packages/whale-api-client/src/api/oracles.ts 22.22% <0.00%> (-77.78%) ⬇️
...hale-api/src/module.api/legacy.subgraph.service.ts 16.12% <0.00%> (-70.17%) ⬇️
apps/libs/caches/src/LegacyCache.ts 33.33% <0.00%> (-66.67%) ⬇️
apps/whale-api/src/module.api/legacy.controller.ts 36.84% <0.00%> (-63.16%) ⬇️
packages/whale-api-client/src/whale.rpc.client.ts 50.00% <0.00%> (-50.00%) ⬇️
...-api-client/src/errors/client.timeout.exception.ts 50.00% <0.00%> (-50.00%) ⬇️
apps/whale-api/src/module.api/oracle.controller.ts 44.00% <0.00%> (-44.00%) ⬇️
... and 111 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link

github-actions bot commented Nov 19, 2022 •
edited
Loading

Docker build preview for jellyfish/apps is ready!

Built with commit 541b9e8

  • ghcr.io/jellyfishsdk/legacy-api:pr-1863
  • ghcr.io/jellyfishsdk/ocean-api:pr-1863
  • ghcr.io/jellyfishsdk/playground-api:pr-1863
  • ghcr.io/jellyfishsdk/status-api:pr-1863
  • ghcr.io/jellyfishsdk/whale-api:pr-1863

You can also get an immutable image with the commit hash

  • ghcr.io/jellyfishsdk/legacy-api:541b9e8190363518b8dd0ebe7514885011e2abb5
  • ghcr.io/jellyfishsdk/ocean-api:541b9e8190363518b8dd0ebe7514885011e2abb5
  • ghcr.io/jellyfishsdk/playground-api:541b9e8190363518b8dd0ebe7514885011e2abb5
  • ghcr.io/jellyfishsdk/status-api:541b9e8190363518b8dd0ebe7514885011e2abb5
  • ghcr.io/jellyfishsdk/whale-api:541b9e8190363518b8dd0ebe7514885011e2abb5

@imkven imkven mentioned this pull request Nov 22, 2022
Closed
imkven
imkven previously approved these changes Nov 30, 2022
View reviewed changes
Copy link
Contributor

@imkven imkven left a comment

Choose a reason for hiding this comment

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

LGTM

infinia-yzl
infinia-yzl suggested changes Nov 30, 2022
View reviewed changes
Copy link
Contributor

@infinia-yzl infinia-yzl left a comment

Choose a reason for hiding this comment

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

Please add the tests mentioned in the call we had earlier. Other than that, LGTM!

infinia-yzl
infinia-yzl previously approved these changes Nov 30, 2022
View reviewed changes
Copy link
Contributor

@infinia-yzl infinia-yzl left a comment

Choose a reason for hiding this comment

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

Mostly nitpicks. Other than that, LGTM.

packages/jellyfish-transaction-builder/__tests__/txn/txn_builder_masternode_update.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-transaction-builder/__tests__/txn/txn_builder_masternode_update.test.ts Outdated Show resolved Hide resolved
packages/jellyfish-transaction-builder/src/txn/txn_builder_masternode.ts Outdated Show resolved Hide resolved
packages/jellyfish-transaction-builder/__tests__/txn/txn_builder_masternode_update.test.ts Outdated Show resolved Hide resolved
imkven
imkven reviewed Dec 1, 2022
View reviewed changes
Copy link
Contributor

@imkven imkven left a comment

Choose a reason for hiding this comment

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

LGTM. There have other comments from other stakeholders.

@marktanrj marktanrj mentioned this pull request Dec 1, 2022
Closed
imkven
imkven previously approved these changes Dec 1, 2022
View reviewed changes
@marktanrj
Copy link
Contributor Author

Give me a moment, trying to see if I can make the vout change/utxo amount more deterministic based on feedback from @delphk

@marktanrj marktanrj dismissed stale reviews from imkven and infinia-yzl via 0574935 December 1, 2022 04:21
@marktanrj
Copy link
Contributor Author

Ready for review again 😅

@marktanrj marktanrj mentioned this pull request Dec 1, 2022
Merged
@marktanrj
Copy link
Contributor Author

Shifted transaction builder code to #1894

fuxingloh
fuxingloh approved these changes Dec 1, 2022
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.

LGTM, this part is clear

@fuxingloh fuxingloh merged commit 9b418aa into main Dec 1, 2022
@fuxingloh fuxingloh deleted the marktanrj/updatemasternode-dftx branch December 1, 2022 07:03
fuxingloh pushed a commit that referenced this pull request Jan 14, 2023
@marktanrj @infinia-yzl
feat(jellyfish-transaction-builder): Add updatemasternode transaction…
0dc5628 
… builder (#1894)

<!--  Thanks for sending a pull request! -->

#### What this PR does / why we need it:
- updatemasternode transaction builder

#### Which issue(s) does this PR fixes?:
<!--
(Optional) Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->
Fixes part of #1842

#### Additional comments?:
- #1863 needed for typings
- <del> #1892 needed for createDeFiTx </del>

Co-authored-by: Isaac Yong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@delphk delphk delphk left review comments

@fuxingloh fuxingloh fuxingloh approved these changes

@infinia-yzl infinia-yzl infinia-yzl left review comments

@imkven imkven imkven left review comments

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

@canonbrother canonbrother Awaiting requested review from canonbrother canonbrother is a code owner

@kodemon kodemon Awaiting requested review from kodemon

@eli-lim eli-lim Awaiting requested review from eli-lim

@cwkang1998 cwkang1998 Awaiting requested review from cwkang1998

@helloscoopa helloscoopa Awaiting requested review from helloscoopa

Assignees
No one assigned
Labels
area/packages kind/feature New feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@marktanrj @infinia-yzl @imkven @fuxingloh @cwkang1998 @delphk