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

CosmosOrchestrationAccount's amountToCoin returns incorrect denom when presented a Brand #10449

Open
0xpatrickdev opened this issue Nov 11, 2024 · 2 comments
Labels
bug Something isn't working needs-design

Comments

@0xpatrickdev
Copy link
Member

Describe the bug

CosmosOrchestrationAccount's amountToCoin / coerceCoin / coerceDenom helpers do not account for issuing/host chain logic.

Example:

// actual
helper.amountToCoin(ISTbrand) => ({ denom: 'uist', amount: 1n })

// expected
helper.amountToCoin(ISTbrand, holdingChain) => ({ denom: 'ibc/uisthash', amount: 1n })

Most tests use DenomArg instead of AmountArg, as we are waiting on #9752, #9966, #9967, so this path wasn't well trodden.

To Reproduce

/packages/orchestration/test/exos/cosmos-orchestration-account.test.ts has a test that mentions this

Expected behavior

The helper should return the denom from the perspective of the holding chain. IOW, the host chain for the ICA.

@0xpatrickdev 0xpatrickdev added the bug Something isn't working label Nov 11, 2024
@0xpatrickdev 0xpatrickdev changed the title CosmosOrchestrationAccount's amountToCoin does not account for CosmosOrchestrationAccount's amountToCoin returns incorrect denom when presented a Brand Nov 11, 2024
0xpatrickdev added a commit to 0xpatrickdev/agoric-sdk that referenced this issue Nov 12, 2024
this test can no longer rely on the IST brand being available, so we
 - added checks to ensure brands are accepted for `.transfer` and `.send`
 - filed Agoric#10449 since this surfaced a bug in `amountToCoin`
 - use Moolah issuer for "no denom for brand" failure path tests
0xpatrickdev added a commit that referenced this issue Nov 26, 2024
this test can no longer rely on the IST brand being available, so we
 - added checks to ensure brands are accepted for `.transfer` and `.send`
 - filed #10449 since this surfaced a bug in `amountToCoin`
 - use Moolah issuer for "no denom for brand" failure path tests
0xpatrickdev added a commit that referenced this issue Nov 26, 2024
this test can no longer rely on the IST brand being available, so we
 - added checks to ensure brands are accepted for `.transfer` and `.send`
 - filed #10449 since this surfaced a bug in `amountToCoin`
 - use Moolah issuer for "no denom for brand" failure path tests
0xpatrickdev added a commit that referenced this issue Nov 26, 2024
this test can no longer rely on the IST brand being available, so we
 - added checks to ensure brands are accepted for `.transfer` and `.send`
 - filed #10449 since this surfaced a bug in `amountToCoin`
 - use Moolah issuer for "no denom for brand" failure path tests
0xpatrickdev added a commit that referenced this issue Nov 28, 2024
this test can no longer rely on the IST brand being available, so we
 - added checks to ensure brands are accepted for `.transfer` and `.send`
 - filed #10449 since this surfaced a bug in `amountToCoin`
 - use Moolah issuer for "no denom for brand" failure path tests
0xpatrickdev added a commit that referenced this issue Nov 28, 2024
this test can no longer rely on the IST brand being available, so we
 - added checks to ensure brands are accepted for `.transfer` and `.send`
 - filed #10449 since this surfaced a bug in `amountToCoin`
 - use Moolah issuer for "no denom for brand" failure path tests
0xpatrickdev added a commit that referenced this issue Nov 29, 2024
this test can no longer rely on the IST brand being available, so we
 - added checks to ensure brands are accepted for `.transfer` and `.send`
 - filed #10449 since this surfaced a bug in `amountToCoin`
 - use Moolah issuer for "no denom for brand" failure path tests
@dckc
Copy link
Member

dckc commented Nov 30, 2024

Thinking about the impact of this bug... it impacts all APIs that call amountToCoin, right?

A quick audit shows lots of high-profile methods:

  • delegate()
  • redelegate()
  • send()
  • sendAll()
  • transfer()

cc @mitdralla

@0xpatrickdev
Copy link
Member Author

0xpatrickdev commented Nov 30, 2024

The impact is that users cannot present IST as a brand and have it mean ibc/uistoncosmos, ibc/uistonosmosis. They can still use the DenomArg part of AmountArg for the methods mentioned above.

I'm wondering if #10588 indirectly addresses the issue. Namely:

https://github.com/Agoric/agoric-sdk/pull/10588/files#diff-194a6213416d31d45303da4244422e90fc7efd61014154ea6380926ee2eaab19R459-R460

This would indicate this is not the expected behavior:

// expected
helper.amountToCoin(ISTbrand, holdingChain) => ({ denom: 'ibc/uisthash', amount: 1n })

Instead, it doesn't seem we have a good story about brands for OrchAccounts. Perhaps, we should drop support for brands in this exo and only accept DenomArg.

0xpatrickdev added a commit that referenced this issue Dec 3, 2024
this test can no longer rely on the IST brand being available, so we
 - added checks to ensure brands are accepted for `.transfer` and `.send`. marked as failing for now
 - filed #10449 since this surfaced a bug in `amountToCoin`
 - use Moolah issuer for "no denom for brand" failure path tests
0xpatrickdev added a commit that referenced this issue Dec 3, 2024
this test can no longer rely on the IST brand being available, so we
 - added checks to ensure brands are accepted for `.transfer` and `.send`. marked as failing for now
 - filed #10449 since this surfaced a bug in `amountToCoin`
 - use Moolah issuer for "no denom for brand" failure path tests
0xpatrickdev added a commit that referenced this issue Dec 3, 2024
this test can no longer rely on the IST brand being available, so we
 - added checks to ensure brands are accepted for `.transfer` and `.send`. marked as failing for now
 - filed #10449 since this surfaced a bug in `amountToCoin`
 - use Moolah issuer for "no denom for brand" failure path tests
0xpatrickdev added a commit that referenced this issue Dec 3, 2024
this test can no longer rely on the IST brand being available, so we
 - added checks to ensure brands are accepted for `.transfer` and `.send`. marked as failing for now
 - filed #10449 since this surfaced a bug in `amountToCoin`
 - use Moolah issuer for "no denom for brand" failure path tests
0xpatrickdev added a commit that referenced this issue Dec 3, 2024
this test can no longer rely on the IST brand being available, so we
 - added checks to ensure brands are accepted for `.transfer` and `.send`. marked as failing for now
 - filed #10449 since this surfaced a bug in `amountToCoin`
 - use Moolah issuer for "no denom for brand" failure path tests
0xpatrickdev added a commit that referenced this issue Dec 3, 2024
this test can no longer rely on the IST brand being available, so we
 - added checks to ensure brands are accepted for `.transfer` and `.send`. marked as failing for now
 - filed #10449 since this surfaced a bug in `amountToCoin`
 - use Moolah issuer for "no denom for brand" failure path tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-design
Projects
None yet
Development

No branches or pull requests

2 participants