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

Using/Teaching zone.exo instead of Far --wip-- #89

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

amessbee
Copy link
Contributor

@amessbee amessbee commented Aug 6, 2024

This adds code to contract file (and related deps to package.json) to use zone.exo to replace Far. Note that this is only for teaching purpose at the moment and does not make resulting contract upgradable because zone and baggage are created inside start function. The upgradability will require changing start signature which I am avoiding at the moment.

Ref(s):
Agoric/documentation#1033

@amessbee amessbee requested review from erights and dckc August 6, 2024 12:18
@amessbee
Copy link
Contributor Author

amessbee commented Aug 6, 2024

This can't be merged until #78 due to node version requirement for zone. @dckc

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.

baggage is the 3rd arg of start. Let's go ahead and do it that way. I think it simplifies things considerably.

I'm not confident that mixing u16 and u12 dependencies is reliable.

contract/src/offer-up.contract.js Outdated Show resolved Hide resolved
contract/src/offer-up.contract.js Outdated Show resolved Hide resolved
contract/src/offer-up.contract.js Outdated Show resolved Hide resolved
"@agoric/zoe": "^0.26.3-u12.0",
"@agoric/zone": "^0.3.0-u16.1",
Copy link
Member

Choose a reason for hiding this comment

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

What impact does this have on the contract bundle size? Does it introduce duplicate dependencies?

Other dependencies in this dapp (zoe, ertp) look like they're from u12. IME, mixing u12 and u16 is asking for trouble.

I know dapp-agoric-basics uses u14: Agoric/dapp-agoric-basics@817bc8a

Are you familiar with the fragility of dependency versions?

It looks like @Jovonni managed to use u16 in dapp-orchestration-basics, though it took some troubleshooting. See item 9 in particular.

contract/src/offer-up.contract.js Show resolved Hide resolved
contract/src/offer-up.contract.js Outdated Show resolved Hide resolved
contract/package.json Outdated Show resolved Hide resolved
@dckc
Copy link
Member

dckc commented Aug 7, 2024

I gather recent symptoms include a problem when starting the ui: a blank page and a console error:

tame-console.js:159  TypeError: Cannot read properties of undefined (reading 'subscribeAfter')

@amessbee amessbee changed the title Using/Teaching zone.exo instead of Far Using/Teaching zone.exo instead of Far --wip-- Jan 8, 2025
@amessbee amessbee marked this pull request as ready for review January 8, 2025 10:51
@amessbee amessbee force-pushed the ms/replace-Far-with-zone.exo branch from eb2d98b to b5d3dea Compare January 8, 2025 10:53
// Mark the publicFacet Far, i.e. reachable from outside the contract
const publicFacet = Far('Items Public Facet', {
// Use zone.exo to make a publicFacet suitable for use by remote callers.
const publicFacet = zone.exo('Items Public Facet', undefined, {
makeTradeInvitation,
Copy link
Member

Choose a reason for hiding this comment

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

I suggest inlining makeTradeInvitation here.

  1. That's the more usual idiom.
  2. I think exo methods have to use concise method syntax.

Copy link
Member

Choose a reason for hiding this comment

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

I think exo methods have to use concise method syntax.

"have to" is a bit too strong, but is directionally right. And exo singleton such as this one does not need to refer to this. Therefore, arrow functions happen to work. But they are not idiomatic and should not be. Methods of exo classes and exo class kits generally do need to refer to their this, and so must use concise method syntax. Also, methods in JavaScript class syntax are necessarily in concise method syntax. So for uniformity, I strongly recommend that even for exo singletons, methods use only concise method syntax.

@amessbee amessbee force-pushed the ms/replace-Far-with-zone.exo branch from 00ba31c to fe3e965 Compare January 9, 2025 11:00
@amessbee amessbee force-pushed the ms/replace-Far-with-zone.exo branch from 1bc346c to 1be91de Compare January 9, 2025 11:42
Comment on lines -133 to -137
* Make an invitation to trade for items.
*
* Proposal Keywords used in offers using these invitations:
* - give: `Price`
* - want: `Items`
Copy link
Member

Choose a reason for hiding this comment

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

any particular reason to drop this bit of documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, don't remember removing them at all, will put them back.

Also, currently there are a lot of type errors in the CI which is bothering me a bit. Let me know if you can find time to help there.

amessbee and others added 2 commits January 10, 2025 08:36
Co-authored-by: Dan Connolly <connolly@agoric.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants