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

refactor(kmarshal): simplify makeStandinPromise #9475

Merged
merged 8 commits into from
Jul 3, 2024

Conversation

erights
Copy link
Member

@erights erights commented Jun 9, 2024

closes: #9434
refs: #XXXX

Description

This removes a kludge needed to accommodate an ancient version of endo. The kludge should no longer be needed, and the remaining code can be simpler.

However, we cannot actually merge this PR to master until we know we do not need to worry about this ancient version of endo.

  • identify what version of endo fixed the problem that the kludge addressed
  • figure out whether that should still inhibit merging this simplification.

Altogether, the kludge is small and cheap, so there's nothing urgent about merging this PR. We should wait until we're confident about the compat risk.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

none

Upgrade Considerations

none

@erights erights self-assigned this Jun 9, 2024
Copy link

cloudflare-workers-and-pages bot commented Jun 9, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 48782ba
Status: ✅  Deploy successful!
Preview URL: https://c8a7b7b4.agoric-sdk.pages.dev
Branch Preview URL: https://markm-9434-simplify-makestan.agoric-sdk.pages.dev

View logs

@erights erights marked this pull request as ready for review June 9, 2024 05:27
@erights erights requested a review from turadg June 9, 2024 05:27
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.

However, we cannot actually merge this PR to master until we know we do not need to worry about this ancient version of endo.

Agreed. The code change looks good but I won't approve for master until questions are answered.

@erights erights requested a review from warner June 27, 2024 19:33
@mhofman
Copy link
Member

mhofman commented Jul 3, 2024

Since both master and the released code depends on newer endo version, I believe it's safe to merge this.

@erights erights requested a review from turadg July 3, 2024 03:34
@erights
Copy link
Member Author

erights commented Jul 3, 2024

However, we cannot actually merge this PR to master until we know we do not need to worry about this ancient version of endo.

Agreed. The code change looks good but I won't approve for master until questions are answered.

Hi @turadg , is @mhofman 's answer adequate to approve?

@erights erights added the automerge:squash Automatically squash merge label Jul 3, 2024
@mergify mergify bot merged commit 00afafb into master Jul 3, 2024
77 checks passed
@mergify mergify bot deleted the markm-9434-simplify-makeStandinPromise branch July 3, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge technical-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove makeStringStandinPromise
3 participants