Implementation of construction/payloads and construction/combine endpoints #275

asimm241 · 2020-10-02T23:29:10Z

construction/payloads
-implment construction/payloads endpoint
-unit test coveratge
construction/combine
-implment construction/combine endpoint
-unit test coveratge

codecov · 2020-10-02T23:33:06Z

Codecov Report

Merging #275 into master will increase coverage by 0.35%.
The diff coverage is 78.37%.

@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
+ Coverage   65.16%   65.52%   +0.35%     
==========================================
  Files          53       53              
  Lines        3801     3910     +109     
  Branches      649      672      +23     
==========================================
+ Hits         2477     2562      +85     
- Misses       1321     1345      +24     
  Partials        3        3

Impacted Files	Coverage Δ
src/api/rosetta-constants.ts	`100.00% <ø> (ø)`
src/api/routes/rosetta/construction.ts	`75.69% <73.80%> (-1.23%)`	⬇️
src/rosetta-helpers.ts	`73.95% <92.59%> (+2.67%)`	⬆️

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 8612ea8...66648c5. Read the comment docs.

src/api/routes/rosetta/construction.ts

asimm241 · 2020-10-07T17:24:50Z

@zone117x Fixed all the issues and updated PR.

zone117x · 2020-10-07T17:33:46Z

src/api/routes/rosetta/construction.ts

+        amount: new BN(amount),
+        fee: new BN(fees),
+        publicKeys: publicKeysStrings,
+        numSignatures: 2,


This appears to only allow for 2-of-2 multisig transactions, it should support N-of-M

reedrosenbluth · 2020-10-07T18:28:11Z

src/api/routes/rosetta/construction.ts

+        amount: new BN(amount),
+        fee: new BN(fees),
+        publicKeys: publicKeysStrings,
+        numSignatures: publicKeys.length,


did you intend for publicKeys.length to be the value here? This will mean that all of the keys associated with publicKeys will have to eventually sign this tx...

typically the creator of a multi-sig transaction is able to specify what this value should be.

I'm assuming the caller is only specifying keys whose signatures are required. It means the caller is the creator of mulit-sig/single-sig transaction here. I can not find any other param in rosetta which will indicate signatures and public keys can be different.

@asimm241 can you verify that other Rosetta implementations do not support m-of-n multisig transactions?

@zone117x I haven't found any implementation to support multi-sig for rosetta in other blockchains. I have found this question https://community.rosetta-api.org/t/construction-api-and-multi-sig-accounts/208. This states that they are targeting m-of-n multi-sig for rosetta v1.5.0

Keeping this in view should we postpone the support of multi-sig.

Moreover, BTW rosetta docs do not require public keys in payloads request but makeUnsignedTokenTransferTransaction requires a public key(s), is there any other way to create a transaction without specifying the public key(s)?

This states that they are targeting m-of-n multi-sig for rosetta v1.5.0. Keeping this in view should we postpone the support of multi-sig.

We can postpone m-of-n until then 👍

rosetta docs do not require public keys in payloads request but makeUnsignedTokenTransferTransaction requires a public key(s)

It appears to be not-required to allow support for blockchains that do not need public keys. Is there anywhere that states the endpoint must work when public keys are not provided?

No, I haven't encountered anything like that so far, I'm searching for it though, I'm also thinking of asking a question on their community forum. Let's see.

src/tests-rosetta/api.ts

src/api/routes/rosetta/construction.ts

src/tests-rosetta/api.ts

zone117x · 2020-10-09T13:18:39Z

src/api/routes/rosetta/construction.ts

+
+    if (publicKeys.length !== 1) {
+      //TODO support multi-sig in the future.
+      res.status(400).json(RosettaErrors.needOnePublicKey);


Do other rosetta implementations do the same thing when there are more than 1 public keys?

Other implementations that I have looked at don't require a public key to build an unsigned transaction.

Is there a way we can create a transaction without specifying the public key?
if No, then we need to update metadata and preprocess API to incorporate the public key.
as per these coinbase/mesh-specifications#46, coinbase/mesh-specifications#45

Here is the PR containing those changes. #288

PR #288 merged. This PR needs a rebase to fix the conflicting file and then looks good to merge 👍

zone117x · 2020-10-09T13:19:01Z

src/api/routes/rosetta/construction.ts

+      return;
+    }
+
+    if (signatures.length !== 1) res.status(400).json(RosettaErrors.needOnlyOneSignature);


Same question here -- is this what other Rosetta implementations do?

Yes!
https://github.com/iotexproject/iotex-core-rosetta-gateway/blob/master/services/construction.go#L65

Alright, we should create a github issue for tracking multi-sig support once the Rosetta spec adds it

Created issue #289

…a path to constants

… path

…openapi

add test cases for multisignature for combine and payloads endpoint

blockstack-devops · 2020-10-12T21:08:47Z

🎉 This PR is included in version 0.26.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

asimm241 mentioned this pull request Oct 6, 2020

Tests for Rosetta account/ and mempool/ endpoints #273

Closed

zone117x reviewed Oct 6, 2020

View reviewed changes

src/api/routes/rosetta/construction.ts Outdated Show resolved Hide resolved

zone117x reviewed Oct 6, 2020

View reviewed changes

src/api/routes/rosetta/construction.ts Outdated Show resolved Hide resolved

zone117x reviewed Oct 6, 2020

View reviewed changes

src/api/routes/rosetta/construction.ts Show resolved Hide resolved

zone117x reviewed Oct 7, 2020

View reviewed changes

asimm241 force-pushed the feat/rosetta-construction-payloads-combine branch from e40b9a5 to 7a00e7a Compare October 7, 2020 17:53

reedrosenbluth reviewed Oct 7, 2020

View reviewed changes

zone117x reviewed Oct 7, 2020

View reviewed changes

src/tests-rosetta/api.ts Outdated Show resolved Hide resolved

zone117x reviewed Oct 7, 2020

View reviewed changes

src/api/routes/rosetta/construction.ts Outdated Show resolved Hide resolved

zone117x reviewed Oct 8, 2020

View reviewed changes

src/tests-rosetta/api.ts Outdated Show resolved Hide resolved

zone117x reviewed Oct 9, 2020

View reviewed changes

zone117x mentioned this pull request Oct 12, 2020

[Rosetta] Implement mult-sig support #289

Closed

zone117x added the rosetta label Oct 12, 2020

asimm241 added 12 commits October 12, 2020 20:24

docs: add schema files for rosetta construction/payloads endpoint

b7507f7

chore: add rosetta construction/payloads empty endpoint and add schem…

03870a7

…a path to constants

feat: implement rosetta construction/payloads endpoint

8f4b626

test: add tests for rosetta construction/payloads endpoint

d5d5400

docs: add schema files for rosetta constructin/combine endpoint

5f141f4

chore: add empty rosetta construction/combine endpoint and add schema…

ef84b7f

… path

feat: add implementation of rosetta construction/combine endpoint

0e435ae

test: add tests for rosetta construction/combine endpoint

3fbea76

docs: add construction/payloads and construction/combine endpoint in …

af52bf4

…openapi

fix: add support for multi signature in payloads endpoint

c92fdc0

add test cases for multisignature for combine and payloads endpoint

fix: fixed number of multisignature

64808b0

refactor: removed support for multi-sig from combine and payloads

66648c5

asimm241 force-pushed the feat/rosetta-construction-payloads-combine branch from b3b477e to 66648c5 Compare October 12, 2020 15:44

asimm241 requested a review from zone117x October 12, 2020 18:12

asimm241 requested a review from reedrosenbluth October 12, 2020 18:12

zone117x approved these changes Oct 12, 2020

View reviewed changes

zone117x merged commit aa73382 into hirosystems:master Oct 12, 2020

blockstack-devops added the released label Oct 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of construction/payloads and construction/combine endpoints #275

Implementation of construction/payloads and construction/combine endpoints #275

asimm241 commented Oct 2, 2020 •

edited

Loading

codecov bot commented Oct 2, 2020 •

edited

Loading

asimm241 commented Oct 7, 2020

zone117x Oct 7, 2020

reedrosenbluth Oct 7, 2020

asimm241 Oct 7, 2020

zone117x Oct 7, 2020

asimm241 Oct 8, 2020

zone117x Oct 8, 2020

asimm241 Oct 8, 2020 •

edited

Loading

zone117x Oct 9, 2020

asimm241 Oct 9, 2020 •

edited

Loading

asimm241 Oct 12, 2020

zone117x Oct 12, 2020

zone117x Oct 9, 2020

asimm241 Oct 9, 2020

zone117x Oct 9, 2020

zone117x Oct 12, 2020

blockstack-devops commented Oct 12, 2020

Implementation of construction/payloads and construction/combine endpoints #275

Implementation of construction/payloads and construction/combine endpoints #275

Conversation

asimm241 commented Oct 2, 2020 • edited Loading

codecov bot commented Oct 2, 2020 • edited Loading

Codecov Report

asimm241 commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asimm241 Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asimm241 Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blockstack-devops commented Oct 12, 2020

asimm241 commented Oct 2, 2020 •

edited

Loading

codecov bot commented Oct 2, 2020 •

edited

Loading

asimm241 Oct 8, 2020 •

edited

Loading

asimm241 Oct 9, 2020 •

edited

Loading