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

fix: wallet connection and chainStorageWatcher #63

Merged
merged 2 commits into from
Feb 3, 2024

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented Feb 3, 2024

  • there was a regression in test(ui): ensure title and connect wallet button render #57 that broke makeAgoricChainStorageWatcher - it needs to take an apiUrl instead of rpc
  • makeAgoricWalletConnection needs an rpc to work, and an api endpoint, so this change brings it up to date with the new @agoric/rpc changes
  • also ensures there's only one yarn.lock in the workspace

Tested manually as it'd be a bit of work to add testing with keplr. A more cost effective test in the interim could be a UI banner that shows if the instance was found (API only), and a test to see if it's present. That would validate chainStorageWatcher is working (and, the contract install). Edit: Seem something to this effect is tracked here: #46

- there was a regression in #57 that broke `makeAgoricChainStorageWatcher` - it needs to take an apiUrl instead of rpc
- `makeAgoricWalletConnection` needs an rpc to work, and an api endpoint, so this change brings it up to date with the new @agoric/rpc changes
@0xpatrickdev 0xpatrickdev mentioned this pull request Feb 3, 2024
@dckc dckc changed the title fix: wallet connection and chainStorageWathcher fix: wallet connection and chainStorageWatcher Feb 3, 2024
@0xpatrickdev
Copy link
Member Author

P.S. if you are testing manually, voteLatestProposalAndWait seems to consistently fail - out of gas.

Revisiting efforts to fix that upstream: Agoric/agoric-3-proposals#73

@dckc
Copy link
Member

dckc commented Feb 3, 2024

...

  • also ensures there's only one yarn.lock in the workspace

any particular reason? I thought separating them was on purpose.

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.

This looks like it can't hurt.

API: 'http://localhost:1317',
};

const watcher = makeAgoricChainStorageWatcher(ENDPOINTS.API, 'agoriclocal');
Copy link
Member

Choose a reason for hiding this comment

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

@0xpatrickdev
Copy link
Member Author

...

  • also ensures there's only one yarn.lock in the workspace

any particular reason? I thought separating them was on purpose.

It's my interpretation after reading this article. When I was updating the @agoric/web-components (from the workspace root) I kept observing it having no effect on the ui lock file.

@dckc
Copy link
Member

dckc commented Feb 3, 2024

I don't think we want to use code from the contract package in the UI nor vice versa. And I'm pretty sure we want to be able to use new shiny stuff in the ui before issues that would complicate compatibility between the contract and the chain get addressed. But I'm OK dealing with all that outside this PR.

@0xpatrickdev
Copy link
Member Author

I don't think we want to use code from the contract package in the UI nor vice versa. And I'm pretty sure we want to be able to use new shiny stuff in the ui before issues that would complicate compatibility between the contract and the chain get addressed.

In yarn classic I believe there's a nohoist key that can be parameterized in workspaces in package.json to achieve this. In more recent versions of yarn, there's a nmHoistingLimits param that goes in .yarnrc.yml.

@0xpatrickdev 0xpatrickdev merged commit 1f74994 into main Feb 3, 2024
1 check passed
@0xpatrickdev 0xpatrickdev deleted the pc/fix-wallet-connection branch February 3, 2024 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants