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

9063 init chainInfo #9477

Merged
merged 6 commits into from
Jun 10, 2024
Merged

9063 init chainInfo #9477

merged 6 commits into from
Jun 10, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Jun 10, 2024

refs: #9063

Description

Progress on,

To be closed when all the names in the checklist are present.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

Updated test setup

Upgrade Considerations

Not yet deployed

Copy link

cloudflare-workers-and-pages bot commented Jun 10, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5d4b6fd
Status: ✅  Deploy successful!
Preview URL: https://2ea7b05b.agoric-sdk.pages.dev
Branch Preview URL: https://9063-init-chaininfo.agoric-sdk.pages.dev

View logs

@turadg turadg marked this pull request as ready for review June 10, 2024 14:38
@turadg turadg requested review from dckc and 0xpatrickdev June 10, 2024 14:50
@turadg turadg added the force:integration Force integration tests to run on PR label Jun 10, 2024
Comment on lines +15 to +17
"ava": {
"concurrency": 1,
"serial": true,
Copy link
Member

Choose a reason for hiding this comment

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

@0xpatrickdev you mentioned using serial in the starship work; apparently that's pretty normal for this sort of thing.

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'm not sure about some details around publishNameHubs

@@ -16,7 +16,6 @@ const makeDefaultTestContext = async t => {
);
return swingsetTestKit;
};

Copy link
Member

Choose a reason for hiding this comment

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

stray/left-over edit?

Copy link
Member Author

Choose a reason for hiding this comment

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

spacing for readability

Comment on lines -62 to +66
const current = await wd.getCurrentWalletRecord();
const latest = await wd.getLatestUpdateRecord();
const current = wd.getCurrentWalletRecord();
const latest = wd.getLatestUpdateRecord();
Copy link
Member

Choose a reason for hiding this comment

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

synchronous IO feels funny. But I guess the wallet driver API is that it doesn't do IO here?

Copy link
Member Author

Choose a reason for hiding this comment

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

right, what it's reading is local

@@ -17,7 +17,6 @@ const makeDefaultTestContext = async t => {
);
return swingsetTestKit;
};

Copy link
Member

Choose a reason for hiding this comment

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

another stray / leftover edit?

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

Comment on lines 69 to 70
* stakeBld: Installation<
* import('../../src/examples/stakeBld.contract.js').start
Copy link
Member

Choose a reason for hiding this comment

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

that would be a Promise<Installation<...>>, right?

In fact, why declare a stakeBld installation here at all? it's not used.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks. leftover from an earlier state. I'll remove

@@ -86,6 +108,12 @@ export const getManifestForOrchestration = (_powers, { orchestrationRef }) => ({
orchestrationVat: 'orchestrationVat',
},
},
[initChainInfo.name]: {
consume: {
agoricNames: true,
Copy link
Member

Choose a reason for hiding this comment

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

it's harmless, but why is agoricNames here?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks. leftover from an earlier state. I'll remove

* @deprecated use reflectAgoricNamesToChainStorage
* @deprecated use publishAgoricNamesToChainStorage
Copy link
Member

Choose a reason for hiding this comment

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

that prompted me to realize: adding chain under agoricNames doesn't automatically publish it to vstorage. We need to call E(agoricNames).publishNameHubs(..., ['chain']).

Want to add a test that the chain info is visible from vstorage?

Copy link
Member Author

Choose a reason for hiding this comment

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

that explains the trouble I was running into.

what happens when a new chain is added, do we need to call publishNameHubs again? I kind of expected agoricNames would auto-update its vstorage using the onUpdate callback. Should it?

Copy link
Member Author

@turadg turadg Jun 10, 2024

Choose a reason for hiding this comment

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

confirmed:

const recorderKit =
await E(vatBoard).makePublishingRecorderKit(kindNode);
kindAdmin.onUpdate(recorderKit.recorder);
}),

Doesn't adding chain to agoricNamesReserved get this auto-updater? I"ll add an explicit test

Copy link
Member

Choose a reason for hiding this comment

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

Once we call E(agoricNames).publishNameHubs(..., ['chain']), each time a chain is added, the whole list of entries in vstorage will get updated. But just doing E(agoricNamesAdmin).provideChild('chain') isn't enough to get the updates started for the new namehub.

Comment on lines 36 to 38
chain: {
agoric: 'Agoric chain info',
},
Copy link
Member

Choose a reason for hiding this comment

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

Is this used? It suggests that agoricNames had a chain child all along. It might hide the problem that we need to call E(agoricNames).publishNameHubs(..., ['chain']) when running the orchestration-proposal.js core eval.

Copy link
Member Author

@turadg turadg Jun 10, 2024

Choose a reason for hiding this comment

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

It suggests that agoricNames had a chain child all along

I was on the fence about this but the js doesn't say anything about bootstrap,

* We reserve these keys in name hubs.
*

If it means only reserved before the chain boots, I can take this out but I'd want to include new documentation and probably move these core/utils.js things that are meant only for bootstrap into a module that makes that clear. Please advise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note, this is the change that was necessary to make publishNameHubs set up an onUpdate to publish each new entry to vstorage.

{
"interestRateValue": 1000,
"interchainAssetOptions": {
"denom": "ibc/toyatom",
Copy link
Member

Choose a reason for hiding this comment

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

Calling this ibc/toyatom / ATOM will present an issue when using this config for e2e testing with sim chains and relayers.

Is it straightforward to call this something generic like STAKE to prevent the collision?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will require a lot of other things to change. I'll see if they fit easily into this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

The wallet driver depends on that right now,

agoricNamesRemotes.brand.ATOM || Fail`ATOM missing from agoricNames`;

This was the status quo so I consider changing it out of scope. Many tests use that so it's worth handling in its own PR or in one that will break because of it.

"@agoric/builders/scripts/vats/init-network.js",
"@agoric/builders/scripts/vats/init-localchain.js",
"@agoric/builders/scripts/vats/init-transfer.js",
"@agoric/builders/scripts/vats/init-transfer.js",
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
"@agoric/builders/scripts/vats/init-transfer.js",

Comment on lines +82 to +85
if (!chainStorage) {
console.warn('no chain storage, not registering chain info');
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

sigh; here's hoping we can get rid of this before long

Far('chain info writer', {
write(entries) {
// chainInfo has no cap data but we need to marshal bigints
const marshalData = makeMarshal(_val => Fail`data only`);
Copy link
Member

Choose a reason for hiding this comment

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

👍 interesting... no round trip thru board

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Jun 10, 2024
@mergify mergify bot merged commit 1dcae57 into master Jun 10, 2024
71 checks passed
@mergify mergify bot deleted the 9063-init-chainInfo branch June 10, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants