-
Notifications
You must be signed in to change notification settings - Fork 212
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
test: verify purse balances are updated after smartWallet upgrade #8787
Conversation
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.
Some
"type": "module", | ||
"license": "Apache-2.0", | ||
"dependencies": { | ||
"@agoric/synthetic-chain": "^0.0.3", |
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.
if you use 0.0.4-2
then you don't need core-eval-support.js
and most of performActions.js
.
you'll still need eval.sh
because you're making an extra submission distinct from the agoricProposal
one.
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.
core-eval-support.js
is gone, but I couldn't find definitions of anything in performActions in @synthetic-chain. How/where should I have searched?
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.
3836e70a
takes us back to 0.0.3, so I'm going to assume for now that the suggestions about reducing performActions.js
won't apply until a later branch.
a3p-integration/proposals/b:wallet-factory2/submission/sendInvite-permit.json
Outdated
Show resolved
Hide resolved
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.
No blockers, but the code can be simpler to reduce maintenance cost and be a better example
/** Provide access to the outside world via context. */ | ||
const makeTestContext = async () => { | ||
// assume filenames don't overlap | ||
const bundleAssets = makeFakeWebCache('submission'); |
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.
if you're going to keep this part of performActions.ts
then consider renaming the submission directory and this string that's looking for it.
But I think you could simplify this file to,
import { evalBundles } from '@agoric/synthetic-chain/src/lib/core-eval.js`;
evalBundles('zcfProber-submission');
Oh, I guess there's already support for specifying a directory path. 🤦
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 reran yarn; yarn build
, but I still don't see '@agoric/synthetic-chain/src/lib/core-eval.js';
. I looked in a3p as well, and it doesn't appear to be in packages/synthetic-chain/src/lib
, and I've just pulled again.
Sorry, I asked for a re-review, but I hadn't submitted my comments. Apologies. |
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 moved the proposal from ./submission to ./invite-submission.
"type": "module", | ||
"license": "Apache-2.0", | ||
"dependencies": { | ||
"@agoric/synthetic-chain": "^0.0.3", |
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.
3836e70a
takes us back to 0.0.3, so I'm going to assume for now that the suggestions about reducing performActions.js
won't apply until a later branch.
c1f32d9
to
4ce0658
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.
Why is this a new layer? Why are the tests not added to the chain software upgrade layer which already performs the wallet factory upgrade as a core proposal?
3e13f5b
to
71a2b39
Compare
The main core-eval submission here is the wallet upgrade, which is specified in | ||
package.json and built in ./submission. |
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.
not true anymore but not a blocker.
The wallet upgrade is part of the upgrade-14 "upgradeInfo"
The core-eval is invoked from the test, which then verifies that the details are | ||
written to vstore. | ||
|
||
https://github.com/Agoric/agoric-3-proposals/issues/82 will provide a better way |
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.
lots of room for improvement :)
77dc118
to
5e98f9f
Compare
929c333
to
0be12d4
Compare
0be12d4
to
d06b176
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.
LGTM besides the unexplained logging change to the smart wallet
@@ -0,0 +1,12 @@ | |||
This core-eval installs upgrades for the wallet and Zoe. This test verifies that |
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.
Probably should remove the first sentence since the wallet factory and zoe upgrades are part of the chain software upgrade now.
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.
Please revise the whole readme. I think it was moved from a README for the earlier governance proposal but now it's scoped to just this core-eval used for testRepairs
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.
Here's another attempt.
These files enable a test of the walletFactory changes in upgrade-14, by
verifying that upgraded wallets that aren't backed by vbanks can still add
assets, in this case an invitation.
(sendInvite.tpl) to produce the .js file that will be submitted. | ||
|
||
The core-eval is invoked from the test, which then verifies that the details | ||
were written to vstore. |
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.
were written to vstore. | |
were written to vstorage. |
const replaceAddressInFile = async (string, fileName, replacement) => { | ||
const scriptBuffer = await readFile(`${SUBMISSION_DIR}/${fileName}.tpl`); |
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.
Nit: the order of arguments is surprising. Also why hard code SUBMISSION_DIR
instead of expecting it in the input fileName
?
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 suggestions.
trace(`Found ${brandToPurses.values()} purse(s) for ${address}`); | ||
trace(`Found purse(s) for ${address}`); |
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.
Is this change to debug logging in the smart wallet meant to go into a "testing" PR?
It's also a change I don't see on master.
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.
please revert
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.
done
I simplified testRepairs.ts substantially (and dropped all the added dependencies) after getting it to work cross-platform. It's worth another look. |
integration cleanup
add a CoreEval proposal to a3p-integration
Re-add a CoreEval proposal to a3p-integration
Clarify a3p-integration
* feat(a3p-integration): add core eval in test * fixup! feat(a3p-integration): add core eval in test * Nudge * test: reduce race sensitivity --------- Co-authored-by: Turadg Aleahmad <turadg@agoric.com>
) * test: verify purse balances are updated after smartWallet upgrade * chore: helpful suggestions from review * refactor: testRepairs to Ava test * chore: make test work on master * chore: rearrange files, repair merge conflict * chore: rename * chore: adapt wallet a3p test to new core-eval style * fixup --------- Co-authored-by: Turadg Aleahmad <turadg@agoric.com> Co-authored-by: Mathieu Hofman <mathieu@agoric.com>
44ae7cc
to
6d9459d
Compare
closes: #8293
Description
Verify that purse balances are updated after smartWallet upgrade in an a3p-integration test.
Security Considerations
None.
Scaling Considerations
None
Documentation Considerations
None
Testing/Upgrade Considerations
This is a test that upgrading SmartWallet maintains the ability to update vstorage for purses.