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

more RUN remnants #5913

Merged
merged 2 commits into from
Aug 9, 2022
Merged

more RUN remnants #5913

merged 2 commits into from
Aug 9, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented Aug 8, 2022

Description

Some internal references remained to RUN. This changes those.

It also adds board ids for STABLE as an alternative to RUN. Rather than removing the latter it deprecates so dapps don't break. (those that use https://github.com/search?q=org%3AAgoric+RUN_ISSUER_BOARD_ID&type=code )

Once those are converted to use STABLE we can remove the deprecated in #5605

Security Considerations

--

Documentation Considerations

--

Testing Considerations

Should not affect tests.

@turadg turadg requested a review from dckc August 8, 2022 18:03
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.

good work.

Please consider conventional commits for breaking changes. (not critical)

Comment on lines +596 to +597
['STABLE_ISSUER_BOARD_ID', stableIssuer],
['STABLE_BRAND_BOARD_ID', stableBrand],
Copy link
Member

Choose a reason for hiding this comment

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

things like this really should be looked up from a namehub such as E(home).agoricNames (see also #4819), so adding features to configureVaultFactoryUI seems like the wrong direction. Not critical, I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Easy enough to remove when that work is done. Meanwhile I don't want to slow down the dapps moving off RUN_*

zcf.makeInvitation(offerHandler, 'make RUNstake'),
zcf.makeInvitation(offerHandler, 'make stakeFactory'),
Copy link
Member

Choose a reason for hiding this comment

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

Good catch. This change is part of the visible contract API, so it's more important than many of the others. Please consider separating it out as a separate commit marked as a breaking change fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Visible to end users but not read by machines, correct? I.e. I don't believe this change breaks any commitments of the API.
https://github.com/Agoric/agoric-sdk/blob/master/packages/zoe/src/contractFacet/types.js#L93-L97

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 it would make sense for a machine to select among invitations from an installation based on description. This is discussed in #2230 for example:

Just knowing the instance and description should be enough to choose an invitation to use.

return 'Your RUNstake is closed; thank you for your business.';
return 'Your stakeFactory is closed; thank you for your business.';
Copy link
Member

Choose a reason for hiding this comment

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

Messages like this don't play a critical security role like invitation descriptions, but strictly speaking, this is also a breaking contract API change.

Copy link
Member Author

Choose a reason for hiding this comment

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

The API just says that it returns a string. Anything depending on parsing that string is making unwarranted assumptions about the API.

Copy link
Member

Choose a reason for hiding this comment

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

We have quite a few tests that depend on the contents of these strings. That seemed like an unwarranted assumption to me too, but that seems to be the current design. I guess it doesn't matter that much...

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Aug 8, 2022
@mergify mergify bot merged commit 87b7afd into master Aug 9, 2022
@mergify mergify bot deleted the ta/unrun branch August 9, 2022 00:04
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants