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: add mint-tokens endpoint for p2sh wallet #312

Merged
merged 0 commits into from
Jul 12, 2023

Conversation

alexruzenhack
Copy link
Contributor

@alexruzenhack alexruzenhack commented Jun 22, 2023

Acceptance Criteria

  • add mint-tokens endpoint for p2sh wallet: /wallet/p2sh/tx-proposal/mint-tokens

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

Closes: #302

@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

❗ No coverage uploaded for pull request base (feat/multisig-for-bridge@d9d8a82). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 8d12182 differs from pull request most recent head 36be128. Consider uploading reports for the commit 36be128 to get more accurate results

@@                     Coverage Diff                     @@
##             feat/multisig-for-bridge     #312   +/-   ##
===========================================================
  Coverage                            ?   87.17%           
===========================================================
  Files                               ?       35           
  Lines                               ?     1372           
  Branches                            ?      276           
===========================================================
  Hits                                ?     1196           
  Misses                              ?      155           
  Partials                            ?       21           

@alexruzenhack alexruzenhack force-pushed the feat/create-mint-endpoint branch 2 times, most recently from 0c8fc2e to cdc0a2d Compare June 22, 2023 20:40
}

const mintTokenTransaction = await req.wallet.prepareMintTokensData(
token,

Check failure

Code scanning / CodeQL

NoSQL database query built from user-controlled sources (experimental)

(Experimental) This may be a database query that depends on [a user-provided value](1). Identified using machine learning.
__tests__/__fixtures__/http-fixtures.js Outdated Show resolved Hide resolved
__tests__/mint-tokens.test.js Outdated Show resolved Hide resolved
src/controllers/wallet/p2sh/tx-proposal.controller.js Outdated Show resolved Hide resolved
@alexruzenhack alexruzenhack changed the base branch from dev to feat/multisig-for-bridge July 7, 2023 19:22
@alexruzenhack alexruzenhack marked this pull request as ready for review July 7, 2023 19:24
src/api-docs.js Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
{
"name": "hathor-wallet-headless",
"version": "0.22.0-rc1",
"version": "0.22.0-rc2",
Copy link
Member

Choose a reason for hiding this comment

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

Why are you changing this? Shouldn't you change the lib version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with rebase.

Copy link
Contributor Author

@alexruzenhack alexruzenhack Jul 12, 2023

Choose a reason for hiding this comment

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

See:

"version": "0.22.0-rc3",

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "hathor-wallet-headless",
"version": "0.22.0-rc1",
"version": "0.22.0-rc2",
Copy link
Member

Choose a reason for hiding this comment

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

Why are you changing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with rebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See:

"version": "0.22.0-rc3",

src/api-docs.js Outdated
@@ -6,7 +6,7 @@ const apiDoc = {
info: {
title: 'Headless Hathor Wallet API',
description: 'This wallet is fully controlled through an HTTP API.',
version: '0.22.0-rc1',
version: '0.22.0-rc3',
Copy link
Member

Choose a reason for hiding this comment

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

Why change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with rebase.

Copy link
Contributor Author

@alexruzenhack alexruzenhack Jul 12, 2023

Choose a reason for hiding this comment

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

See:

version: '0.22.0-rc3',

src/api-docs.js Outdated
},
address: {
type: 'string',
description: 'Optional destination address of the minted tokens.'
Copy link
Member

Choose a reason for hiding this comment

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

This is not optional, it's in the required array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It is optional. I changed the required definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See:

required: ['token', 'amount'],

src/api-docs.js Outdated
'application/json': {
examples: {
error: {
summary: 'Insuficient amount of tokens',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
summary: 'Insuficient amount of tokens',
summary: 'Insufficient amount of tokens',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See:

summary: 'Insufficient amount of tokens',

'/mint-tokens',
body('token').isString().notEmpty(),
body('amount').isInt({ min: 1 }).toInt(),
body('address').isString().notEmpty().optional(),
Copy link
Member

Choose a reason for hiding this comment

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

Is it optional? The api docs description says it isn't.

But if the idea is to copy what we have in the current mint-tokens api, then it should be optional and the api docs must be fixed (the current mint tokens api docs as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was fixed in the api docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment: #312 (comment)

@alexruzenhack alexruzenhack merged commit 36be128 into feat/multisig-for-bridge Jul 12, 2023
2 checks passed
@alexruzenhack alexruzenhack deleted the feat/create-mint-endpoint branch July 12, 2023 18:03
@alexruzenhack alexruzenhack restored the feat/create-mint-endpoint branch July 14, 2023 17:08
@alexruzenhack alexruzenhack deleted the feat/create-mint-endpoint branch July 14, 2023 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants