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

test of revising chain info #9552

Merged
merged 4 commits into from
Jun 21, 2024
Merged

test of revising chain info #9552

merged 4 commits into from
Jun 21, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Jun 21, 2024

refs: #8896

Description

Extracted from #9534 to focus that PR on multichain and lighten the review. Also it had merge conflicts with master that this resolves.

Security Considerations

nothing new

Scaling Considerations

no

Documentation Considerations

none

Testing Considerations

new coverage

Upgrade Considerations

none

@turadg turadg requested review from dckc and 0xpatrickdev June 21, 2024 14:56
Copy link

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

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2f625b9
Status: ✅  Deploy successful!
Preview URL: https://ad7bc8a1.agoric-sdk.pages.dev
Branch Preview URL: https://ta-revise-chain-info.agoric-sdk.pages.dev

View logs

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Jun 21, 2024
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'd rather not suggest that we know how to handle changes to chainId.

Comment on lines 260 to 261
const agoricAfter = await EV(agoricNames).lookup('chain', 'agoric');
t.like(agoricAfter, { chainId: 'agoric-4' });
Copy link
Member

Choose a reason for hiding this comment

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

Do we have requirements to handle changing a chainId change for a well-established chain? That's the sort of "earthquake" event that I think the market doesn't really contemplate. It would mean something like abandoning all the tokens that have ever gone over IBC to or from agoric-3. I'd rather not have a test that suggests that we handle this sort of thing. I'd much rather stick to "append only" changes to chain info (for a somewhat hand-wavy notion of append).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we have a need to revise chain_id for Starship because it will have different ids for the names.

Since Starship is an E2E test, I'm fine relying on that actual test for whether the chain ids update as needed and taking this out of the test here that could be read as a product spec. I'll remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Ah. Yes, I look forward to supporting a chainId other than agoric-3 in the starship context. I'd expect it to go into agoricNames once as agoric-4 in the first place, though.

@turadg turadg added automerge:rebase Automatically rebase updates, then merge and removed automerge:rebase Automatically rebase updates, then merge labels Jun 21, 2024
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.

good stuff

@mergify mergify bot merged commit 01a1123 into master Jun 21, 2024
77 checks passed
@mergify mergify bot deleted the ta/revise-chain-info branch June 21, 2024 17:49
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants