-
Notifications
You must be signed in to change notification settings - Fork 9
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: multichain-testing enhancements #108
base: main
Are you sure you want to change the base?
Conversation
3208269
to
b2b674b
Compare
@@ -249,7 +249,6 @@ test('orca-multichain.test style usage', async t => { | |||
const wallets = await ensureAccounts(tools.agd.keys); | |||
t.deepEqual(wallets, { | |||
agoric: 'agoric1v8qxguqqjtfyfwqr8ln2wlu858vkx4860jzf6g', | |||
cosmoshub: 'agoric17th0tvrzmwc2fqpeneuwmrwcm8armyjlgmypuf', |
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 we are removing cosmoshub?
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.
The OG setup had 3 chains: agoric, cosmoshub(aka gaia) and osmosis. This meant running at least three relayers - agoric<>osmosis, agoric<>cosmoshub, osmosis<>cosmoshub. By reducing the multichain testing setup to only two chains, we have also reduced the number of relayers to 1. As a result, I have also removed cosmoshub related changes from code (especially multichain tests which fail because cosmoshub isn't present anymore).
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.
But aren't we then compromising testing with cosmoshub if we are just removing it to save our resources/time?
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.
How so? Whatever we test between two chains is just extrapolated to a set of three chains.
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 didn't get it. Provided the test is same but if it's happening on different chains then we should treat it something different. Outcome of test can be different on different chains, or no?
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.
What is the goal of our test? Is it to generally test multi-chain stuff with any chain?
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.
So let's say the tests work for a<>b chains and a<>c chains. We are removing c chain because the a<>b is enough as it does the same testing, right?
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.
We have parametrized tests like
test.serial(makeAccountScenario, 'agoric');
test.serial(makeAccountScenario, 'osmosis');
This used to include cosmoshub as well.
This test basically creates new wallets, provisions them, makes an offer using orch contract, and verifies the wallet has that offer. Basically testing if our orchestration contract offer is received on other chains (from what I am inferring from code). It already does it for osmosis, so IMO an agoric testcase and a non-agoric test cases suffices. We can add as many non-agoric chains (supported) as we like but we don't have to. If it works for osmosis, it should work for others
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.
Hmmmm fair enough
@mujahidkay seems like you are doing two distinct things here. Would it be possible to split into two separate PRs (using low resources + multichain setup)? Would be easier to review IMO. |
e1f6930
to
cc61a71
Compare
b6da776
to
c561468
Compare
@mujahidkay please update the title (make it more specific and focused) and the description. I see a lot of changes related to removing cosmoshub from different places - shouldn't these changes be in resource optimization PR? |
Description
In line with #95 , this PR uses minimal config for multichain testing. The startup time is roughly the same and not the focus of this PR. This PR also adds relevant scripts from agoric-sdk/multichain-testing so that users can run multichain-testing from within this repo.
Relevant documentation update in a follow-up PR (right after this merges)