Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

docs: Update destination Arg Description in ICA transfer #10983

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amessbee
Copy link
Contributor

refs: #9966

Description

Security Considerations

None

Scaling Considerations

None

Documentation Considerations

Documentation for transfer should also be updated if necessary.

@amessbee amessbee added the automerge:rebase Automatically rebase updates, then merge label Feb 11, 2025
@amessbee amessbee requested a review from dckc February 11, 2025 11:03
@amessbee amessbee marked this pull request as ready for review February 11, 2025 11:03
@amessbee amessbee requested a review from a team as a code owner February 11, 2025 11:03
Copy link

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: f675508
Status: ✅  Deploy successful!
Preview URL: https://f0269b7f.agoric-sdk.pages.dev
Branch Preview URL: https://ms-update-ica-transfer-docs.agoric-sdk.pages.dev

View logs

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.

I need to think this over...

@@ -193,8 +193,9 @@ export interface OrchestrationAccountCommon {
* Transfer an amount to another account, typically on another chain.
* The promise settles when the transfer is complete.
* @param amount - the amount to transfer. Can be provided as pure data using denoms or as ERTP Amounts.
* @param destination - the account to transfer the amount to.
* @param [opts] - an optional memo to include with the transfer, which could drive custom PFM behavior, and timeout parameters
* @param destination - the account to transfer the amount to. The denom in the destination arg has to be
Copy link
Member

Choose a reason for hiding this comment

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

oops! the denom is in the amount, not the destination

* @param destination - the account to transfer the amount to.
* @param [opts] - an optional memo to include with the transfer, which could drive custom PFM behavior, and timeout parameters
* @param destination - the account to transfer the amount to. The denom in the destination arg has to be
* registered with `chainHub` via {@link `registerAsset`} API.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* registered with `chainHub` via {@link `registerAsset`} API.
* registered with `chainHub` via {@link `registerAsset`} method.

* @param destination - the account to transfer the amount to.
* @param [opts] - an optional memo to include with the transfer, which could drive custom PFM behavior, and timeout parameters
* @param destination - the account to transfer the amount to. The denom in the destination arg has to be
* registered with `chainHub` via {@link `registerAsset`} API.
Copy link
Member

Choose a reason for hiding this comment

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

This link doesn't seem to work.

you might try {@link ChainHub}, after checking that ChainHub is imported.
Hm. It might not be imported. It's sort of a lower level concept than OrchestrationAccount.
I should think this over a bit.

@turadg turadg self-requested a review February 11, 2025 19:28
# for free to join this conversation on GitHub. Already have an account? # 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