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

handle multiple econgov invitations due to ec changes #131

Merged
merged 2 commits into from
Oct 3, 2024

Conversation

rabi-siddique
Copy link
Contributor

@rabi-siddique rabi-siddique commented Sep 19, 2024

closes: #132

This PR changes the inferInvitationStatus function to be able to handle multiple invitations from the economicCommittee and economicCommitteeCharter contract.
This is done by filtering the invitations on the basis of the contract instance, which is updated whenever the new invitations are sent

Copy link

cloudflare-workers-and-pages bot commented Sep 19, 2024

Deploying dapp-econ-gov with  Cloudflare Pages  Cloudflare Pages

Latest commit: d3360c2
Status: ✅  Deploy successful!
Preview URL: https://be05fb2a.dapp-econ-gov.pages.dev
Branch Preview URL: https://rs-fraz-ec-changes.dapp-econ-gov.pages.dev

View logs

Comment on lines 569 to 570
const idA = a[0].split('-')[1];
const idB = b[0].split('-')[1];
Copy link
Member

Choose a reason for hiding this comment

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

offerIds aren't guaranteed to be of the form xxx-number. They can be any string or number.

This code has to handle offerIds from other clients.

I think filtering on instance will find the invitation you're looking for.

@frazarshad frazarshad changed the title EC Changes handle multiple econgov invitations due to ec changes Oct 1, 2024
@frazarshad frazarshad marked this pull request as ready for review October 1, 2024 16:06
@frazarshad frazarshad requested a review from dckc October 1, 2024 16:07
@frazarshad frazarshad self-assigned this Oct 1, 2024
Copy link
Contributor

@samsiegart samsiegart left a comment

Choose a reason for hiding this comment

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

  • Tested on emerynet with EC account and it said I could vote 👍
  • Tested on mainnet with non-EC account and it said I couldn't vote 👍

Had one non-blocking question and one non-blocking suggestion but otherwise LGTM

@@ -6,6 +6,9 @@ export default mergeConfig(
viteConfig,
defineConfig({
test: {
deps: {
inline: ['@agoric/rpc'],
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://vitest.dev/config/#server-deps-inline

Vite will process inlined modules. This could be helpful to handle packages that ship .js in ESM format (that Node can't handle).

There were some agoric dependencies which imported file like so

import x from 'foo';

but vitest by default would not accept anything other than

import x from 'foo.js';

this config fixed that

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks good to know. I wonder if this is fixed in the latest @agoric/rpc versions with Agoric/ui-kit#76, this dapp is using some pretty old versions. But that's a separate issue.

import { inferInvitationStatus } from './wallet';
import { LoadStatus } from './rpc';

const createMock = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rename this to createMockWallet or something similar, and instead of mocks below, mockWallet or such.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for writing tests!

@frazarshad frazarshad merged commit ac03280 into main Oct 3, 2024
3 checks passed
@frazarshad frazarshad deleted the rs-fraz-ec-changes branch October 3, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When membership is rotated, dapp-econ-gov doesn't find the right invitation
4 participants