Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Rebase smartWallet (#8488) to release-mainnet1B branch #8609

Closed
wants to merge 3 commits into from
Closed

Rebase smartWallet (#8488) to release-mainnet1B branch #8609

wants to merge 3 commits into from

Conversation

Chris-Hibbert
Copy link
Contributor

refs: #8488

Description

Rebase #8488 on release-mainnet1B

Security Considerations

None

Scaling Considerations

None

Documentation Considerations

N/A

Testing Considerations

Verify that the tests all pass.

Upgrade Considerations

It's all about preparing for upgrade.

@Chris-Hibbert Chris-Hibbert added wallet test next-release about next agoric-sdk or endo release labels Dec 4, 2023
@Chris-Hibbert Chris-Hibbert self-assigned this Dec 4, 2023
@Chris-Hibbert
Copy link
Contributor Author

Many tests are failing because the fix for fakeVirtualSupport to handle watchedPromises doesn't work here. The vrm.allocateNextID() call doesn't exit here.

@mhofman mhofman changed the base branch from release-mainnet1B to dev-upgrade-wallet-factory-2 December 4, 2023 23:47
mhofman
mhofman reviewed Dec 4, 2023
View reviewed changes
packages/boot/test/bootstrapTests/liquidation.ts Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the boot package doesn't exist in the release branch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx. dropped.

packages/inter-protocol/src/price/roundsManager.js Outdated
Comment on lines 438 to 440
if (status.lastReportedRound >= roundId)
if (status.lastReportedRound >= roundId) {
return 'cannot report on previous rounds';
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an unnecessary change for the release branch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

packages/swingset-liveslots/src/watchedPromises.js Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance we can avoid these non-runtime changes in the release branch ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

packages/swingset-liveslots/src/types.js Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance we can avoid these non-runtime changes in the release branch ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

@Chris-Hibbert Chris-Hibbert changed the base branch from dev-upgrade-wallet-factory-2 to release-mainnet1B December 14, 2023 19:21
mhofman
mhofman reviewed Dec 14, 2023
View reviewed changes
packages/smart-wallet/package.json Outdated
@@ -26,6 +26,7 @@
"dependencies": {
"@agoric/assert": "^0.6.1-u11wf.0",
"@agoric/casting": "^0.4.3-u13.0",
"@agoric/deploy-script-support": "^0.10.3",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the release branch, this is the version of deploy-script-support.

Suggested change
"@agoric/deploy-script-support": "^0.10.3",
"@agoric/deploy-script-support": "^0.10.4-u13.0",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Chris-Hibbert Chris-Hibbert changed the base branch from release-mainnet1B to 8618-fakeVirtual-watchedPromises December 14, 2023 23:52
@Chris-Hibbert Chris-Hibbert mentioned this pull request Dec 15, 2023
Closed
5 tasks
turadg
turadg reviewed Dec 15, 2023
View reviewed changes
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed #8488 (review) .

I think that should land first. Once it lands, the commit should be rebased here. I will be on PTO at that point.

Base automatically changed from 8618-fakeVirtual-watchedPromises to dev-upgrade-wallet-factory-2 December 15, 2023 23:04
@Chris-Hibbert Chris-Hibbert added the force:integration Force integration tests to run on PR label Jan 2, 2024
@Chris-Hibbert Chris-Hibbert force-pushed the 8445-addWatcher-release branch 5 times, most recently from 3641e4a to 6ec6f9e Compare January 3, 2024 00:56
mhofman
mhofman previously requested changes Jan 4, 2024
View reviewed changes
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there are some spurious changes unrelated to wallet factory in this PR

packages/inter-protocol/src/price/roundsManager.js Outdated
@@ -437,6 +437,7 @@ export const prepareRoundsManagerKit = baggage =>

if (status.lastReportedRound >= roundId)
return 'cannot report on previous rounds';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an unnecessary change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

packages/inter-protocol/src/vaultFactory/vaultManager.js Outdated
@@ -1035,7 +1035,7 @@ export const prepareVaultManagerKit = (
state.collateralBrand,
);
if (!storedCollateralQuote)
throw Fail`lockOraclePrices called before a collateral quote was available`;
throw Fail`lockOraclePrices called before a collateral quote was available for ${state.collateralBrand}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the vault factory getting upgraded in this ? If not, this change should not be part of the dev branch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropped

turadg and others added 2 commits January 4, 2024 13:48
@turadg @Chris-Hibbert
feat: smartWallet verstion 2 with watchedPromises
73bed28 
pulled offers.js and payments.js into smartWallet.js as they shared
plenty of state that needs to be durable in order to be callable from
the watchedPromise.

build an upgrade proposal; tested in
Agoric/agoric-3-proposals#34
@Chris-Hibbert
chore: clean-ups from review
7c1d677
mhofman
mhofman reviewed Jan 4, 2024
View reviewed changes
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not reviewed the logic, nor if the rebase is faithful, but the scope of modifications is acceptable for release.

@mhofman mhofman dismissed their stale review January 4, 2024 23:40

Addressed

turadg
turadg approved these changes Jan 5, 2024
View reviewed changes
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the commits are the same as master. The reconciliation commit looks okay. There's a merge conflict to solve but I trust it will be safe.

@Chris-Hibbert
Copy link
Contributor Author

Given recent discussions, I will wait for a new release branch before proceeding.

@Chris-Hibbert Chris-Hibbert mentioned this pull request Jan 18, 2024
Merged
@Chris-Hibbert
Copy link
Contributor Author

superseded by #8773.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhofman mhofman mhofman left review comments

@turadg turadg turadg approved these changes

Assignees

@Chris-Hibbert Chris-Hibbert

Labels
force:integration Force integration tests to run on PR next-release about next agoric-sdk or endo release test wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Chris-Hibbert @turadg @mhofman