-
Notifications
You must be signed in to change notification settings - Fork 1
Use Chain ID from process.env in E2E tests #51
base: main
Are you sure you want to change the base?
Conversation
e1b7ca3
to
c6076dc
Compare
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 branch includes this change and has a chain e2e test enabled. By viewing the logs I've verified that we now see the correct chainNetworkId of 9002 .
Should the test fail when the following error is logged?
RuntimeError: VM Exception while processing transaction: revert Incorrect chainId\
If no, why not?
If yes, why didn't it fail? Should we do this?
- Address the test issue, exhibiting a test failure when the VM Exception is raised
- Address the
chainId
issue, and observe that the failing test now passes
10_000, // timeout | ||
10_00_000_000, // gasLimit | ||
1 // gasPrice |
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 are these changed?
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.
Oops this was copied from the chain-setup
in server-wallet
. The important bit is that we don't use 1337
as the chain id.
So this is what is happening:
Some takeaways:
|
5d8fdeb
to
cf8057d
Compare
Since the |
42decbc
to
9ec1050
Compare
I remember we had much discussion in the past about configuration via environment variables. I think we should certainly not be doing that in the server wallet code itself (since we have a configuration object that we pass in that specifies the config). Does it make sense to do it in the e2e tests in this repo? (genuine question) |
I think this is ok because we're only using it in the context of tests. It's just a convenient way to pass a value from the |
9ec1050
to
56a21e1
Compare
56a21e1
to
138d124
Compare
Description
Consistently set the
chainId
toprocess.env.CHAIN_NETWORK_ID
.I noticed this error in the logs:
Upon investigation, I confirmed that we're not actually setting the
networkChainID
properly in various wallet configurations used for e2e-testing.This PR fixes that (and works around this issue).
Testing
This branch includes this change and has a
chain
e2e test enabled. By viewing the logs I've verified that we now see the correctchainNetworkId
of9002
.Checklist:
Code quality
Project management