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.

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

Clean up after smartWalllet incarnation2 repair #10319

Merged
merged 1 commit into from
Oct 23, 2024
Merged

Conversation

Chris-Hibbert
Copy link
Contributor

closes: #9404

Description

In #8445, we repaired legacy smartWallets. This removes the code that ran in upgrade15 that did the cleanup.

Security Considerations

None

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

No tests should be removed.

Upgrade Considerations

There is no hurry to merge this code on chain. If the PR merges soon, it'll be included in upgrade 18.

@Chris-Hibbert Chris-Hibbert added wallet hygiene Tidying up around the house code-style defensive correctness patterns; readability thru consistency contract-upgrade labels Oct 23, 2024
@Chris-Hibbert Chris-Hibbert self-assigned this Oct 23, 2024
@Chris-Hibbert Chris-Hibbert requested a review from a team as a code owner October 23, 2024 00:23
Copy link

cloudflare-workers-and-pages bot commented Oct 23, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 88019c5
Status: ✅  Deploy successful!
Preview URL: https://24691081.agoric-sdk.pages.dev
Branch Preview URL: https://9404-cleanup.agoric-sdk.pages.dev

View logs

@Chris-Hibbert Chris-Hibbert requested a review from turadg October 23, 2024 19:21
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.

Looks right. I confirmed there's no upgradeTo or secreteWallet left after this change

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.

Looks right. I confirmed there's no upgradeTo or secreteWallet left after this change

@Chris-Hibbert Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Oct 23, 2024
@mergify mergify bot merged commit 0d3bd96 into master Oct 23, 2024
90 checks passed
@mergify mergify bot deleted the 9404-cleanup branch October 23, 2024 20:30
@dckc
Copy link
Member

dckc commented Oct 28, 2024

If the PR merges soon, it'll be included in upgrade 18.

how so? I don't see anything relevant in upgrade.go.

@Chris-Hibbert
Copy link
Contributor Author

It was there until 3 days ago. I'll have to add it again.

mergify bot pushed a commit that referenced this pull request Oct 29, 2024
refs: #9404 
refs: #10336
refs: #10319



## Description

#10319 cleaned up some dead code in smartWallets, and expected the changes to be included in Upgrade 18, because the smartWallet proposal was included in `upgrade.go`. #10336 cleaned up proposals that were left over from Upgrade 17. This reinstates the upgrade for wallets.

### Security Considerations

The cleaned up code is benign. it was designed to run once and has served its purpose. Removing it shouldn't affect anything.

### Scaling Considerations

No scaling consequences.

### Documentation Considerations

None

### Testing Considerations

The wallet upgrade was included in A3P-integration testing until three days ago. #10319 has been tested since 5 days ago without incident. removing benign dead code doesn't require extra tests.

### Upgrade Considerations

There is no *requirement* that this change be included in U18, so if it causes problems or gives anyone the willies, it can wait for the next release, but it's ready now.
mujahidkay pushed a commit that referenced this pull request Oct 30, 2024
refs: #9404
refs: #10336
refs: #10319

The cleaned up code is benign. it was designed to run once and has served its purpose. Removing it shouldn't affect anything.

No scaling consequences.

None

The wallet upgrade was included in A3P-integration testing until three days ago. #10319 has been tested since 5 days ago without incident. removing benign dead code doesn't require extra tests.

There is no *requirement* that this change be included in U18, so if it causes problems or gives anyone the willies, it can wait for the next release, but it's ready now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge code-style defensive correctness patterns; readability thru consistency contract-upgrade hygiene Tidying up around the house wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove repairWalletForIncarnation2 from smartWallet
3 participants