-
Notifications
You must be signed in to change notification settings - Fork 206
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
register any chain info #9452
register any chain info #9452
Conversation
Deploying agoric-sdk with Cloudflare Pages
|
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.
oops; I started reviewing without noticing the DRAFT status
@@ -0,0 +1,61 @@ | |||
/** | |||
* @file static declaration of known chain types will allow type support for |
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 expect this as a test fixture, but not a source module.
This info will vary between a3p, devnet, mainnet, etc.
oh. weird. this is just a .ts->.js rename. it was already here.
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.
yeah, I figure it's all prototype code still. though the .ts
was less risky to have in src
because it would never show up in a bundle.
can we leave this here until #8879? I'll put a TODO
/** | ||
* cf https://github.com/cosmos/chain-registry/blob/master/chain.schema.json#L117 | ||
*/ | ||
stakingTokens?: [{ denom: string }]; |
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.
That syntax gave me a strange error about [X]
vs. X[]
. This made it go away:
stakingTokens?: [{ denom: string }]; | |
stakingTokens?: Array<{ denom: string }>; |
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 imagine that's your linter. That is a better type though, because what's here is a tuple rather than an array of arbitrary length. I'll change it.
*/ | ||
|
||
/** @type {any} */ | ||
const anyVal = null; | ||
|
||
// FIXME should be configurable | ||
// ??? could the localchain service offer this? | ||
const mockLocalChainInfo = { |
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.
mockX
looks like test code, not src
code.
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.
it's not test code either; it's an incomplete implementation. just like all the "'fixme'" strings we've had to date
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've put issues at all the fixmes to be sure they'll be addressed
deposit(payment) { | ||
async deposit(payment) { |
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.
orthogonal to this PR: In asyncFlow
discussion yesterday, somebody said the facade methods can't be async.
* @template {CosmosChainInfo} CCI | ||
* @param {CCI} chainInfo |
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.
funky.
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.
request?
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.
sorry; no... it just hand't occurred to me that a type could be parameterized by a whole structure, and not just a primitive value
const chainInfo = chainInfos.has(name) | ||
? chainInfos.get(name) | ||
: // @ts-expect-error may be undefined | ||
wellKnownChainInfo[name]; |
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.
well known chain should come from agoricNames
, yes?
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.
eventually, yes. that's #9063. I'll add a TODO
CosmosChainAccountMethods, | ||
CosmosChainInfo, | ||
IBCMsgTransferOptions, |
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.
This file shouldn't depend on cosmos types, should it?
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.
ideally not, but it already does. I see that as independent cleanup
const result = (await handle()) as Chain<any>; | ||
t.deepEqual(await result.getChainInfo(), mockChainInfo); | ||
|
||
// FIXME allow re-registering. perhaps this should be called `provide`. |
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.
init / register seems like the right thing to me; I wouldn't expect to be able to re-register.
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.
PTAAL; I pushed a change after you started reviewing
message: '"chainInfos" already registered: "mock"', | ||
}); | ||
|
||
// FIXME support running again in the same zone (as will happen after an upgrade) |
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.
FIXME when? oops... I started reviewing without noticing the DRAFT status.
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 requested it, thank you. I think it's okay to land FIXME comments in master until we're ready to publish the package
}), | ||
{ | ||
message: | ||
'key "chainInfos" has already been used in this zone and incarnation', |
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.
darn it; I thought zone.mapStore('chainInfos', ...)
already had provide semantics.
packages/orchestration/src/facade.js
Outdated
const chainInfos = zone.mapStore('chainInfos', { | ||
const chainInfos = makeScalarBigMapStore('chainInfos', { |
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.
did you disconnect it from the zone on purpose?
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.
yes, to reduce state to maintain across upgrades. however the contract registered the chainInfo the first time, it can do again. and in many cases it won't want to because it'll have made it into the upstream wellKnown registry
Co-Authored-By: Dan Connolly <dckc@madmode.com>
Co-Authored-By: Dan Connolly <dckc@madmode.com>
adcc359
to
fd70556
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'm surprised that the way the test makes zcf
passes lint. But I suppose it doesn't matter.
? IcaAccount | ||
: {}) & | ||
CCI extends { | ||
stakingTokens: {}; |
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.
does it matter that stakingTokens
is an Array? should this be...
stakingTokens: {}; | |
stakingTokens: []; |
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.
Is an array accurate? I know this is how chain-registry
has it modeled, but I am skeptical. The bond_denom
field in x/staking
params seems to indicate just a single entry: https://buf.build/cosmos/cosmos-sdk/docs/main:cosmos.staking.v1beta1#cosmos.staking.v1beta1.Params
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.
Yes, it is curious. https://github.com/search?q=repo%3Acosmology-tech%2Fchain-registry+stakingTokens shows only examples of a single element in the array.
I consider this out of scope that will be resolved in already scheduled issues.
|
||
const zone = bootstrap.rootZone; | ||
|
||
const { zcf } = await setUpZoeForTest(); |
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.
does setUpZoeForTest
make a zcf
? I get an error when I try this (in another context).
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.
good catch. I guess the zcf
isn't used in this test. I've changed it to get a working one though,
const { zcf } = await setupZCFTest();
But I don't know why TS didn't catch this. It typed zcf
as any
. It goes away with noImplicitAny: true
but that causes many other linting errors
const handle = orchestrate('mock', {}, async orc => { | ||
return orc.getChain('mock'); |
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.
thinking out loud: passing the chain object outside the context of the orchestrate
callback seems odd somehow. Is that consistent with the asyncFlow
rules? (where do we find those rules?)
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.
IDK. But if it isn't allowed I'd like to see a test where this would break.
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.
LGTM. It seems like #8879 is out of scope for the follow up PR(s) for #9187, but bringing this mocked file to your attention to maybe update in the course of your efforts:
https://github.com/Agoric/agoric-sdk/blob/5659362b247593793bd30a6b726b261587701487/packages/orchestration/src/utils/mockChainInfo.js
allowedMessages: [], | ||
allowedQueries: [], |
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.
Are you planning to use these for generating Orchestration Account facets? Regardless, looking forward to seeing the approach for that in the next iteration.
const [{ denom: bondDenom }] = chainInfo.stakingTokens || [ | ||
{ | ||
denom: null, | ||
}, | ||
]; | ||
assert(bondDenom, 'missing bondDenom'); |
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.
Mentioned here - but i don't expect we'll see an array of staking staking tokens for anyone using cosmos.staking.v1beta1
fd70556
to
6d597fd
Compare
## Description In a recent discussion we noticed that the test wasn't showing a type error where it should have: #9452 (comment) It would have with `noImplicitAny: false` but that pulls in way too many other errors. While investigating I noticed some other things that could be cleaned up and tackled those here. The remaining blocker for `skipLibCheck: false` is the Telescope output in `dist`. The .js files have `@ts-nocheck` but the .d.ts don't. Surprising but not worth more effort. ### Security Considerations n/a, types ### Scaling Considerations n/a, types ### Documentation Considerations none ### Testing Considerations CI ### Upgrade Considerations Potential cherry-pick complexities but we're moving to release based on master
refs: #9187
Description
Progress on #9187
Plus incidental improvements
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations