-
Notifications
You must be signed in to change notification settings - Fork 212
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 clientSupport to not depend on agoric-cli package #7824
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks plausible in most ways, but I see what looks like a couple breaking changes that aren't sufficiently documented.
I'm reluctant to approve anything at this point without an explicit compatibility note (as the template suggests under Documentation Considerations).
* @param {(msg: string) => Error} makeError error constructor | ||
* @returns {(a: string) => Amount<'nat'>} | ||
*/ | ||
export const makeParseAmount = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing this export is a breaking change to the @agoric
package, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the Agoric CLI is a UI tool and no exports are supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple things could make that more clear:
- something in the README.md
- in
package.json
, something like"exports": []
The the README has a bunch of stuff about running an IBC relayer that seems out of place in contexts such as https://www.npmjs.com/package/agoric
d7a6de8
to
4e63f72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer that the agoric
package were explicit about not exporting anything
* @param {(msg: string) => Error} makeError error constructor | ||
* @returns {(a: string) => Amount<'nat'>} | ||
*/ | ||
export const makeParseAmount = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple things could make that more clear:
- something in the README.md
- in
package.json
, something like"exports": []
The the README has a bunch of stuff about running an IBC relayer that seems out of place in contexts such as https://www.npmjs.com/package/agoric
11ab3cb
to
9246f8e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good.
it's only for installing the bin and importing that bin module
9246f8e
to
cf76198
Compare
packages/agoric-cli/package.json
Outdated
"./lib/helpers.js": "./lib/helpers.js", | ||
"./src/helpers.js": "./src/helpers.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lib
variant no longer exists. It's only referenced by the loadgen for compat with older SDK revisions.
"./lib/helpers.js": "./lib/helpers.js", | |
"./src/helpers.js": "./src/helpers.js" | |
"./src/helpers.js": "./src/helpers.js" |
cf76198
to
d089675
Compare
refactor clientSupport to not depend on agoric-cli package
refactor clientSupport to not depend on agoric-cli package
fixes #8281 |
refs: #4645
Description
^^
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations