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

Construction API work #23

Merged
merged 37 commits into from
Aug 7, 2020
Merged

Construction API work #23

merged 37 commits into from
Aug 7, 2020

Conversation

pro-wh
Copy link
Collaborator

@pro-wh pro-wh commented Jul 25, 2020

based on #22

  • phase 1: signing
  • phase 2: workflow tests
  • phase 3: transaction modeling
  • phase 4: payloads and parsing tests

closes #22 because it includes that

@pro-wh pro-wh force-pushed the pro-wh/feature/cons branch from 0727f4f to f3dfcbf Compare July 28, 2020 00:12
@pro-wh pro-wh force-pushed the pro-wh/feature/cons branch from f3dfcbf to 84fca7a Compare July 28, 2020 19:03
@pro-wh pro-wh force-pushed the pro-wh/feature/cons branch from 48dfe0a to 13f15c4 Compare July 30, 2020 18:26
@pro-wh pro-wh marked this pull request as ready for review July 30, 2020 20:30
@pro-wh pro-wh force-pushed the pro-wh/feature/cons branch from 2a2d74c to 5fef825 Compare July 30, 2020 20:31
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Can we also add tests that actually submit transactions to the network and wait for them to be included in a block? Just to make sure that all the transaction hashes are correctly computed.

Also, are there any tests for the construction API provided by the upstream package?

services/construction.go Outdated Show resolved Hide resolved
services/construction.go Outdated Show resolved Hide resolved
services/construction.go Outdated Show resolved Hide resolved
tests/construction-signing/main.go Show resolved Hide resolved
@pro-wh
Copy link
Collaborator Author

pro-wh commented Jul 31, 2020

Can we also add tests that actually submit transactions to the network and wait for them to be included in a block? Just to make sure that all the transaction hashes are correctly computed.

I'll have to see about how to do that.

Also, are there any tests for the construction API provided by the upstream package?

no ):, unless it's undocumented. although I might as well ask on their forum

@pro-wh
Copy link
Collaborator Author

pro-wh commented Jul 31, 2020

@kostko
Copy link
Member

kostko commented Jul 31, 2020

https://community.rosetta-api.org/t/automated-construction-api-testing/120 oh hey, just one day ago

Cool, we should run that as part of our CI flow :-)

@kostko
Copy link
Member

kostko commented Jul 31, 2020

Just note that currently the construction API is using SubmitTx slightly incorrectly (see #24) as it blocks until the transaction gets executed. It may still be fine for tests though. Fixing this requires using Oasis Core master which adds a non-blocking version.

@pro-wh
Copy link
Collaborator Author

pro-wh commented Jul 31, 2020

There are some major backwards incompatibilities in rosetta-cli 0.4.0. I'll file a separate issue to upgrade to that

#25

Copy link
Contributor

@abukosek abukosek left a comment

Choose a reason for hiding this comment

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

Looks great! Just needs support for the automated construction API testing as part of the test script and it will be good to merge :)

pro-wh added 2 commits August 7, 2020 11:02
run rosetta-cli check:construction in CI
rosetta-cli 0.4.0 and construction tests
@pro-wh pro-wh merged commit 248f78d into master Aug 7, 2020
@pro-wh pro-wh deleted the pro-wh/feature/cons branch August 7, 2020 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants