test(e2e): sendAnywhere contract #9898
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deploying agoric-sdk with
|
| Latest commit: |
c3c11be
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f162d01e.agoric-sdk.pages.dev |
| Branch Preview URL: | https://pc-send-anywhere-e2e.agoric-sdk.pages.dev |
0xpatrickdev
force-pushed
the
pc/send-anywhere-e2e
branch
from
6c311db to
e3fbbd8
Compare
- allows us to use brands from vstroage.query(published.agoricNames.brand) with walletDriver (provisionSmartWallet)
0xpatrickdev
force-pushed
the
pc/send-anywhere-e2e
branch
from
e3fbbd8 to
71a2a94
Compare
|
Logs from local runs, ci logs can be found here: https://github.com/Agoric/agoric-sdk/actions/runs/10393409141/job/28780860582?pr=9898#step:12:1889 first run (unsuccessful) - Error: 0uist is smaller than 10uist: insufficient fundssecond run (successful) |
0xpatrickdev
force-pushed
the
pc/send-anywhere-e2e
branch
3 times, most recently
from
801d00e to
b07211b
Compare
0xpatrickdev
commented
Aug 15, 2024
Comment on lines
124
to
132
| const makeRandomValue = (min: number, max: number) => | ||
| BigInt(Math.floor(Math.random() * (max - min + 1)) + min); | ||
| // send 3 offers from each account | ||
| const scenarios = [ | ||
| makeRandomValue(1, 33), | ||
| makeRandomValue(34, 66), | ||
| makeRandomValue(67, 100), | ||
| ]; | ||
| console.log('Scenarios', scenarios); |
There was a problem hiding this comment.
This was to force different amounts to surface something like:
Error: 98uist is smaller than 100uist: insufficient funds'
as mentioned #9901 (comment)
It may no longer make sense to keep around if it reads confusingly
There was a problem hiding this comment.
I didn't think much of it but someone might. Consider a comment about how the change in values prevents mistaking one result for another
turadg
approved these changes
Aug 15, 2024
Comment on lines
16
to
20
| const chains = ['osmosis', 'cosmoshub']; | ||
| const scenarios = Array(2) | ||
| .fill(undefined) | ||
| .flatMap((_, i) => chains.map(c => [c, i])) as [string, number][]; | ||
| const accounts = scenarios.map(args => args.join('')); |
There was a problem hiding this comment.
it took me some minutes to make sense of this (the logic and where the values are used). Consider:
Suggested change
| const chains = ['osmosis', 'cosmoshub']; | |
| const scenarios = Array(2) | |
| .fill(undefined) | |
| .flatMap((_, i) => chains.map(c => [c, i])) as [string, number][]; | |
| const accounts = scenarios.map(args => args.join('')); | |
| const accounts = ['osmosis', 'cosmoshub'].map((name, i) => `${name}${i}`); |
And rather than data-driving with scenarios, just explicitly call each one.
There was a problem hiding this comment.
We're testing with 4 accounts, so I explicitly wrote them out. (the above would just return 2). Acknowledge the readability feedback and implemented your suggestions 🙏
Comment on lines
140
to
142
| for (const [chainName, accountIdx] of scenarios) { | ||
| test.serial(sendAnywhereScenario, chainName, accountIdx); | ||
| } |
There was a problem hiding this comment.
this is more flexible but since it's a test sometimes it's better to be explicit:
Suggested change
| for (const [chainName, accountIdx] of scenarios) { | |
| test.serial(sendAnywhereScenario, chainName, accountIdx); | |
| } | |
| test.serial(sendAnywhereScenario, 'osmosis', 1); | |
| test.serial(sendAnywhereScenario, 'cosmoshub', 2); |
Though I'd also suggest dropping the index number and having a lookup for the test account for each named chain.
| const makeRandomValue = (min: number, max: number) => | ||
| BigInt(Math.floor(Math.random() * (max - min + 1)) + min); | ||
| // send 3 offers from each account | ||
| const scenarios = [ |
There was a problem hiding this comment.
confusing that this whole function executes one scenario, then within it there are three other "scenarios"
maybe "offerAmounts"?
| proposal: { give: { Send: amount } }, | ||
| }); | ||
|
|
||
| // TODO confirm offerResult = undefined |
0xpatrickdev
force-pushed
the
pc/send-anywhere-e2e
branch
from
b07211b to
7d4800c
Compare
until it works on top of new (upgrade-16)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refs: #9193
closes: #9901
Description
Adds an e2e test for
sendAnywhere.contract.js, ensuringzoeTools.localTransfer()andlocalOrchestrationAccount.transfer()are working as expected.Fixes a bug in
zoeTools.transfer()where a vow chain was broken (we usedPromise.all()on an array of vows instead ofvowTools.allVows().Also satisfies this requirement in #9193:
Security Considerations
n/a
Scaling Considerations
n/a
Documentation Considerations
n/a
Testing Considerations
Includes tests for paths where bugs were discovered.
Upgrade Considerations
n/a