feat!: chainHub.getAsset requires holdingChainName parameter
#10588
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0xpatrickdev
force-pushed
the
pc/10445-chain-hub-get-asset
branch
from
dc416d2
to
d43db44
Compare
Deploying agoric-sdk with
|
| Latest commit: |
c12d868
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4e07532d.agoric-sdk.pages.dev |
| Branch Preview URL: | https://pc-10445-chain-hub-get-asset.agoric-sdk.pages.dev |
0xpatrickdev
force-pushed
the
pc/10445-chain-hub-get-asset
branch
3 times, most recently
from
88245f1
to
d280d69
Compare
- to facililate registering the same denom across multiple chains in `ChainHub` - denoms are unique to a chain, but not all chains
- since denoms are only unique to a chain and all chains, require `holdingChainName` parameter for asset info lookups
0xpatrickdev
force-pushed
the
pc/10445-chain-hub-get-asset
branch
from
d280d69
to
c3403e5
Compare
|
Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label. |
0xpatrickdev
force-pushed
the
pc/10445-chain-hub-get-asset
branch
2 times, most recently
from
7cb895e
to
1eed391
Compare
0xpatrickdev
force-pushed
the
pc/10445-chain-hub-get-asset
branch
from
1eed391
to
c12d868
Compare
This seems unrelated to these changes and is likely a flake. |
This was referenced Nov 30, 2024
dckc
approved these changes
Nov 30, 2024
| @@ -146,6 +146,7 @@ export interface Orchestrator { | |||
| IssuingChain extends keyof KnownChains, | |||
| >( | |||
| denom: Denom, | |||
| holdingChainName: HoldingChain, | |||
There was a problem hiding this comment.
why is holdingChainName constrained to a static list of known chains?
OK, I guess I see why for now. Tolerable tech debt, I suppose.
In due course, it seems like getDenomInfo should be parameterized by...
getDenomInfo: <
HoldingChainInfo extends ChainInfo,
IssuingChainInfo extends ChainInfo,
>(
and maybe even the ConnectionInfo between them.
| const actual = orc.getDenomInfo('utoken1'); | ||
| const actual = orc.getDenomInfo( | ||
| 'utoken1', | ||
| // @ts-expect-error 'mock' not a KnownChain |
There was a problem hiding this comment.
doesn't seem like that should be an error
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refs: #10445
Description
IBC Denoms are unique to a chain but not all not all chains. e.g., if
channel-0points toosmosisfor two chains, theuosmodenom will beibc/ED07A3391A112B175915CD8FAF43A2DA8E4790EDE12566649D0C2F97716B8518for both.This change requires callers to specify the
holdingChainNamefor the denom they wish to retrieve information about.Security Considerations
None
Scaling Considerations
None
Documentation Considerations
None
Testing Considerations
Updates existing tests. Motivated from work in #10571 which surfaced this issue.
Upgrade Considerations
Breaking change, but for library code that will be part on NPM or FUSDC release.