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

Implement payment routes #1711

Merged

Conversation

weilbith
Copy link
Contributor

Closes: #1646

Short description

Implements the endpoints to do payments and read the payment history.

This is an "interpretation" of the current Raiden API specification. This means that there are errors defined which do not make sense or are unclear. After
a first quick discussion with the transcore team there will be an issue to discuss this topic.
Furthermore there are the new issues #1708 and #1710 as result of this work. Anyways the goal is to finalize #1646 as fast as possible with a good solution for now.

Definition of Done

  • Steps to manually test the change have been documented
  • Acceptance criteria are met
  • Change has been manually tested by the reviewer (dApp)

@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #1711 into master will increase coverage by 0.00%.
The diff coverage is 90.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1711   +/-   ##
=======================================
  Coverage   95.30%   95.30%           
=======================================
  Files         148      148           
  Lines        5322     5328    +6     
  Branches     1019     1022    +3     
=======================================
+ Hits         5072     5078    +6     
  Misses        203      203           
  Partials       47       47           
Flag Coverage Δ
#dapp 91.48% <ø> (ø)
#sdk 96.85% <90.00%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
raiden-ts/src/raiden.ts 95.61% <90.00%> (-0.20%) ⬇️
raiden-ts/src/utils/types.ts 95.65% <0.00%> (+1.44%) ⬆️

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 19f5e41...0a312e5. Read the comment docs.

@weilbith
Copy link
Contributor Author

The referenced issue for the API spec is this one.

@andrevmatos andrevmatos self-requested a review June 17, 2020 14:44
@andrevmatos andrevmatos removed their assignment Jun 17, 2020
@andrevmatos andrevmatos added cli 🔤 Command-line SDK-based Node.js Client issues sdk 🖥 labels Jun 17, 2020
@weilbith
Copy link
Contributor Author

I'm fixing the failing tests for the amount validation soon. Sorry for that.

weilbith added 3 commits June 18, 2020 13:33
This includes errors for parameter that are not even used by the cli.
Still this makes it complete and use the new feature of the decoding to
also log these kind of errors.
This allows to do transfers as well as getting some part of the transfer
history with a filter. Be aware that the API specification name it
payment what the SDK calls a transfer. The code tries to make the name
transition as good as it can.
The plain message that the PFS returned an error is not helpful for the
user. Therefore the error details get appended in case they exist.
@weilbith weilbith force-pushed the feature/ci-implement-payment-routes branch from 1343ca6 to 69913e8 Compare June 18, 2020 13:17
@weilbith
Copy link
Contributor Author

I rebased on master to get the recent features and solve conflicts. I included small improvements as well. @andrevmatos please have a (final) look.

@weilbith
Copy link
Contributor Author

weilbith commented Jun 18, 2020

@kelsos this error seem to be related to your recent PR. I must admit I can't follow this. For me it looks a bit like the restored cache for the cli does not work well with the new dependencies of the sdk.

@kelsos
Copy link
Contributor

kelsos commented Jun 18, 2020

Any change something went wrong with the package-lock.json on the rebase?

It doesn't make any sense, I just did a new PR on top of masterand it had no errors.

@andrevmatos
Copy link
Contributor

I've seem some of those errors recently. Seems like some flakness on npm's or circleci side. Restarting from Failed usually is enough to get them to pass.

@kelsos
Copy link
Contributor

kelsos commented Jun 18, 2020

That is what I did. Restarted the flow.

@christianbrb
Copy link
Contributor

Is there a flakiness in our tests?

@kelsos
Copy link
Contributor

kelsos commented Jun 19, 2020

Is there a flakiness in our tests?

Tests have been fine, there where a couple of problems with the CI flow though. In this case I am guessing it was package caching and the package installation failed. We also had an issue with the build step failing quite too often that should be fixed now.

@christianbrb
Copy link
Contributor

@kelsos Ok, great :) That's much more relaxing than flaky tests ;)

@weilbith
Copy link
Contributor Author

CI has went through successful now. Could someone have a quick look and give the necessary approval to merge?

token_address: transfer.token,
amount: transfer.value.toString(),
identifier: transfer.paymentId.toNumber(),
secret: '', // FIXME: must be first exposed by SDK (#1708)
Copy link
Contributor

Choose a reason for hiding this comment

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

secret is already exposed

Copy link
Contributor

@andrevmatos andrevmatos left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@weilbith weilbith merged commit c0fc81c into raiden-network:master Jun 22, 2020
@weilbith weilbith deleted the feature/ci-implement-payment-routes branch June 22, 2020 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli 🔤 Command-line SDK-based Node.js Client issues sdk 🖥
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement missing API endpoints for the train demo
4 participants