-
Notifications
You must be signed in to change notification settings - Fork 50
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
chore/56 separate services logic #121
Conversation
import { BlockService } from '../services/block-service'; | ||
import { mapToAccountBalanceResponse } from '../utils/data-mapper'; | ||
|
||
/* eslint-disable camelcase */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think this is necessary
request, | ||
async () => { | ||
const ttlOffset = request.body.options.relative_ttl; | ||
const ttl = await (await constructionService.calculateTtl(request.log, ttlOffset)).toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is there two awaits? I think the one outside the parenthesis is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
test/e2e/network/common.ts
Outdated
@@ -0,0 +1,7 @@ | |||
export const generateNetworkPayload = (blockchain: string, network: string) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no return type in function
test/e2e/utils/test-utils.ts
Outdated
@@ -29,9 +29,9 @@ export const cardanoCliMock: CardanoCli = { | |||
export const setupServer = (database: Pool): FastifyInstance => { | |||
const logger = configLogger(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not used
b21d478
to
02ff90d
Compare
We are not being consistent with the loggeer variable name, some places is used as log.info and others as logger.info |
I think I replaced all the logs to |
@t-dallas all comments fixed! |
ec245ba
to
81830da
Compare
I've run the Docker image build locally: ./scripts/build_dev_docker_images.sh
...
Successfully built f296c61e5dbe
Successfully tagged cardano-rosetta:dev
...
Successfully built f9cc3f12df98
Successfully tagged cardano-rosetta:dev-testnet |
req.log is being used to log Closes: #105
81830da
to
ccf7522
Compare
Description
This PR contains a huge (but safe thanks to the tests) refactor. Due to the fact we followed
openapi-glue
we eneded up with huge modules with a lot of code. This PR tries to fix that.Also, as it was just part of following the same refactor, I introduced the refactor for #105
Sorry this PR got too big and it's not easy to review but it was a little bit cumbersome to create a PR for each module.
This PR depends on #120
Proposed Solution
Important Changes Introduced
Environment config,
TESTS_ENABLED
is no longer needed, we can just set level tosilent
and it will workTesting
yarn test