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

1488 Endo Passable #8774

Closed
wants to merge 52 commits into from
Closed

1488 Endo Passable #8774

wants to merge 52 commits into from

Conversation

turadg
Copy link
Member

@turadg turadg commented Jan 19, 2024

#endo-branch: master

refs: #1488

Description

Adapt to actual Passable type

Reviewers, the bar as yet is not whether to merge to this repo. This PR is to serve as evidence that endojs/endo#2238 can merge and release and agoric-sdk can quickly adopt the new Endo packages.

Once the required Endo code is available by NPM,

  1. bump the Endo packages here to their new releases
  2. remove #endo-branch pragma
  3. verify the Cloudflare job starts passing (it fails now because it doesn't know about #endo-branch)

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

To test locally,

cd $ENDO_REPO
git checkout 1488-Passable-redux
git pull
cd $AGORIC_REPO
scripts/replace-package.sh $ENDO_REPO

Upgrade Considerations

Copy link

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

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5b43819
Status:🚫  Build failed.

View logs

@turadg turadg mentioned this pull request Apr 23, 2024
1 task
@turadg turadg force-pushed the 1488-Endo-Passable branch 4 times, most recently from 98dd81a to 7acec08 Compare April 26, 2024 19:37
@turadg turadg force-pushed the 1488-Endo-Passable branch 3 times, most recently from 9694a37 to d52b5fd Compare April 26, 2024 21:44
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

hope to discuss my comments so far

packages/ERTP/src/types.js Outdated Show resolved Hide resolved
Comment on lines 279 to 283
* @param {Promise<Installation>} installationP
* @param {IssuerKeywordRecord} uncleanIssuerKeywordRecord
* @param {unknown} customTerms
* @param {unknown} privateArgs
* @param {string} instanceLabel
Copy link
Member

Choose a reason for hiding this comment

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

in my work-in-progress docs branch, I used @type ZoeService['startInstance'] here, I think

packages/ERTP/src/amountMath.js Outdated Show resolved Hide resolved
Comment on lines +217 to +209
* @returns {B extends Brand<'nat'>
* ? NatAmount
Copy link
Member

Choose a reason for hiding this comment

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

doesn't AmountMath.make(aNatBrand, ['a set value']) falsify this inference?

packages/ERTP/src/amountMath.js Outdated Show resolved Hide resolved
packages/ERTP/src/amountMath.js Show resolved Hide resolved
@turadg turadg force-pushed the 1488-Endo-Passable branch from 9c25111 to 1403a64 Compare May 2, 2024 22:09
turadg added a commit to endojs/endo that referenced this pull request May 2, 2024
closes: #1488

## Description

Reattempting:
-  #1933

Again:
- Makes `Passable` a generic type instead of `any`.
- Defines overloads for `passStyleOf` to return the actual style.
- Defines the `Key` in terms of `Passable`
- Makes a ton of fixes and suppressions in places that relied on the
previous `any`'s

### Security Considerations

n/a

### Scaling Considerations

n/a

### Documentation Considerations

Better types are better documentation.

### Testing Considerations

- [ ] Agoric/agoric-sdk#8774

### Upgrade Considerations

These changes may cause type errors downstream. I don't think we should
call those breaking change à la semver because they don't affect the
runtime.
@turadg turadg force-pushed the 1488-Endo-Passable branch 2 times, most recently from 487fca0 to 18720f3 Compare May 2, 2024 23:27
@turadg
Copy link
Member Author

turadg commented May 2, 2024

endojs/endo#2238 merged to master so I updated the #endo-branch. I've also rebased onto master and included type coverage before/after measurements.

Once this is green the next steps are to:

  • publish latest Endo
  • update this PR to include those Endo versions
  • get it back to green
  • merge

@turadg turadg force-pushed the 1488-Endo-Passable branch from b6dc0e1 to d4aaa74 Compare May 6, 2024 21:07
@turadg turadg marked this pull request as ready for review May 6, 2024 21:07
@turadg turadg requested a review from dckc May 6, 2024 21:07
@turadg turadg force-pushed the 1488-Endo-Passable branch from d4aaa74 to 09c12d6 Compare May 7, 2024 17:14
turadg added a commit to endojs/endo that referenced this pull request May 7, 2024
## Description

`ScalarKey` is a patterns concept but without a typedef. This defines
it.

Necessary for
Agoric/agoric-sdk#8774 (review)


### Security Considerations

no

### Scaling Considerations

no

### Documentation Considerations

no

### Testing Considerations

new tests

### Compatibility Considerations

no

### Upgrade Considerations

no
@turadg turadg mentioned this pull request May 7, 2024
@turadg turadg added the force:integration Force integration tests to run on PR label May 8, 2024
mergify bot added a commit that referenced this pull request May 8, 2024
This brings Agoric in sync with Endo versions
endojs/endo#2272 which covers a new `Passable`
type and improvements to `pass-style`, `patterns`, and `exo`. This
subsumes changes from #8774
necessary for integration.
@turadg
Copy link
Member Author

turadg commented May 10, 2024

Landed in the sync PR

@turadg turadg closed this May 10, 2024
mergify bot added a commit that referenced this pull request May 10, 2024
## Description

Follow up to #8774

Fix the Board types

### Security Considerations

<!-- Does this change introduce new assumptions or dependencies that, if
violated, could introduce security vulnerabilities? How does this PR
change the boundaries between mutually-suspicious components? What new
authorities are introduced by this change, perhaps by new API calls?
-->

### Scaling Considerations

<!-- Does this change require or encourage significant increase in
consumption of CPU cycles, RAM, on-chain storage, message exchanges, or
other scarce resources? If so, can that be prevented or mitigated? -->

### Documentation Considerations

<!-- Give our docs folks some hints about what needs to be described to
downstream users.

Backwards compatibility: what happens to existing data or deployments
when this code is shipped? Do we need to instruct users to do something
to upgrade their saved data? If there is no upgrade path possible, how
bad will that be for users?

-->

### Testing Considerations

<!-- Every PR should of course come with tests of its own functionality.
What additional tests are still needed beyond those unit tests? How does
this affect CI, other test automation, or the testnet?
-->

### Upgrade Considerations

The one behavior change is to omit non-remotables from the board, but
nothing should have been doing that and if it was this error will help.

Of course that doesn't need to go out in any particular chain upgrade,
but it could to minimize divergence from master until,
- #9252
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants