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

fix: remove addInstance call from add-auction.js #10454

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

frazarshad
Copy link
Contributor

@frazarshad frazarshad commented Nov 12, 2024

closes: #XXXX
refs: #XXXX

Description

removes the call to addInstance made in add-auction.js
This is to stop it from breaking another call to addInstance made in the replace-electorate.js proposal for the same instance

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

Copy link

cloudflare-workers-and-pages bot commented Nov 12, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: d16781f
Status: ✅  Deploy successful!
Preview URL: https://d2a50e0b.agoric-sdk.pages.dev
Branch Preview URL: https://fraz-fix-auctioneer-addinsta.agoric-sdk.pages.dev

View logs

@frazarshad frazarshad added the force:integration Force integration tests to run on PR label Nov 12, 2024
@frazarshad frazarshad marked this pull request as ready for review November 12, 2024 15:27
@frazarshad frazarshad requested a review from a team as a code owner November 12, 2024 15:27
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

I'd prefer to remove the copy in replaceElectorate.js. That version made it necessary to add a dependency between the two proposals. Now that the two proposals are started in a known order in a software upgrade, it makes sense to not bother with that interconnection.

  await E(ecCreatorFacet).addInstance(
    auctionUpgradeNewInstance,
    auctionUpgradeNewGovCreator,
    'auctioneer',
  );

Cleaning that up completely requires changing the auction proposal to not provide auctionUpgradeNewInstance and auctionUpgradeNewGovCreator. I can make that change to the PR if you like. Or we could say that that's a bigger change to the RC, and not worth it. This approach works, and is less code. It just leaves messier proposals behind.

@frazarshad
Copy link
Contributor Author

@Chris-Hibbert imo a less code approach would be preferable for a fix unless you feel strongly about decoupling the proposals

@frazarshad frazarshad added the automerge:rebase Automatically rebase updates, then merge label Nov 12, 2024
@frazarshad frazarshad force-pushed the fraz/fix-auctioneer–addInstance branch from a5937e9 to d16781f Compare November 12, 2024 21:23
@mergify mergify bot merged commit d19b293 into master Nov 12, 2024
81 checks passed
@mergify mergify bot deleted the fraz/fix-auctioneer–addInstance branch November 12, 2024 21:59
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 force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants