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

Scaffold accounts service with connector #6

Merged
merged 5 commits into from
May 5, 2021
Merged

Conversation

wilsonianb
Copy link
Contributor

No description provided.

@wilsonianb wilsonianb force-pushed the bw-connector-accounts branch 9 times, most recently from b0a8827 to d57768e Compare May 4, 2021 06:38
@wilsonianb wilsonianb marked this pull request as ready for review May 4, 2021 14:46
Copy link

@chris-jackson-actionqa chris-jackson-actionqa left a comment

Choose a reason for hiding this comment

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

Looks like we'll be testing against a pre-populated database. No problem with that! However, will there also be test hooks to dynamically create and cleanup accounts and transactions? Seeing it's a new project, maybe add this functionality from the beginning?

Not a blocking comment. Just an observation. Feel free to ignore/resolve.

export type AppContainer = IocContract<AppServices>

export class App {
private koa!: Koa<DefaultState, AppContext>
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than using non-null assertions + boot(), I suggest having an async "constructor" (a static method on App that returns a Promise<App>). The synchronous parts of the constructor can be relegated to the actual (private) constructor, which the static method would call when its done with its async setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


public disabled!: boolean

public assetCode!: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Why assetCode!: and not assetCode: (likewise for the other fields)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got this error without it and used the backend's models as guidance

error TS2564: Property 'assetCode' has no initializer and is not definitely assigned in the constructor

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I guess that's because BaseModel defines the constructor.

@@ -4,4 +4,3 @@ export * from './logger'
export * from './peers'
export * from './router'
export * from './tokens'
export * from './accounts'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove 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.

It's a duplicate of line 1.

@wilsonianb wilsonianb force-pushed the bw-connector-accounts branch 2 times, most recently from 928c2b0 to 7faa0d3 Compare May 4, 2021 22:48
@wilsonianb
Copy link
Contributor Author

@sethalicious I plan on wiping the db after each accounts service test (like here). Is that what you have in mind?

I'm not sure if the tigerbeetle client currently supports resetting the db, so that may require some workarounds in the tests in the short term.

@chris-jackson-actionqa
Copy link

@sethalicious I plan on wiping the db after each accounts service test (like here). Is that what you have in mind?

I'm not sure if the tigerbeetle client currently supports resetting the db, so that may require some workarounds in the tests in the short term.

No, Im not referring to wiping the db. Actually have hooks like "POST /createTestAccount", "POST deleteTestAccount", "POST clearTransactions".

That way we can set up (and clean up) scenarios from the test instead of having to have a pre-populated database. Let the tests create the data and accounts via API calls.

@jorangreef
Copy link

I'm not sure if the tigerbeetle client currently supports resetting the db, so that may require some workarounds in the tests in the short term.

I'm not sure if we should do this at the API layer for the client or via the cli. It could be a dangerous feature if it was accidentally called by the client in production, although we could restrict usage only to config.deployment_environment == .development. We don't have this functionality yet.

@wilsonianb wilsonianb merged commit 1b0b8c9 into main May 5, 2021
@wilsonianb wilsonianb deleted the bw-connector-accounts branch May 5, 2021 15:27
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.

4 participants