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

feat: vote to change swap fee #21

Merged
merged 9 commits into from
Mar 18, 2024
Merged

feat: vote to change swap fee #21

merged 9 commits into from
Mar 18, 2024

Conversation

dckc
Copy link
Member

@dckc dckc commented Mar 7, 2024

refs:

stacked on:

TODO:

  • set up a committee
  • wallet level test of voting, in addition to puppet governance
  • e2e / on-chain testing

@dckc dckc force-pushed the dc-vote-fee branch 4 times, most recently from 82c5038 to b14cc6a Compare March 8, 2024 09:14
@Chris-Hibbert
Copy link
Contributor

Thanks for sharing. This looks like good progress! I didn't see anything to improve.

// import { CONTRACT_ELECTORATE, ParamTypes } from '@agoric/governance';
const CONTRACT_ELECTORATE = 'Electorate';
export const ParamTypes = /** @type {const} */ ({
INVITATION: 'invitation',
Copy link
Member Author

@dckc dckc Mar 8, 2024

Choose a reason for hiding this comment

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

IOU a test to check that these constants are in sync with the imports

Copy link
Member Author

Choose a reason for hiding this comment

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

many / most are tested now

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not import constants.js?

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 rollup.config.mjs that I'm using for core eval scripts so far doesn't grok inter-package module specifiers.
There's a rollup plugin to do that, but I don't (yet) want to give the impression that this is a general module linking approach; for that, folks should use bundleSource via agoric run.

I did write up docs for this approach:

@dckc dckc force-pushed the dc-vote-fee branch 2 times, most recently from 3116ff7 to 8154484 Compare March 12, 2024 03:36
@dckc dckc changed the title feat: vote to change swap fee (WIP) feat: vote to change swap fee Mar 12, 2024
@dckc dckc force-pushed the dc-vote-fee branch 3 times, most recently from ed19e45 to e9f8604 Compare March 12, 2024 05:37
@dckc dckc marked this pull request as ready for review March 12, 2024 05:40
@dckc dckc mentioned this pull request Mar 12, 2024
3 tasks
@dckc dckc requested review from LuqiPan and Chris-Hibbert March 12, 2024 05:51
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

LGTM.

All comments/suggestions are non-blocking


/**
* cf. packages/inter-protocol/src/econCommitteeCharter.js
* TODO: test
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the TODO obsolete? and the one on line 25?

// import { CONTRACT_ELECTORATE, ParamTypes } from '@agoric/governance';
const CONTRACT_ELECTORATE = 'Electorate';
export const ParamTypes = /** @type {const} */ ({
INVITATION: 'invitation',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not import constants.js?

/**
* @param {import('ava').ExecutionContext} t
* @param {MockWallet} wallet
* @param {{ instance: BootstrapPowers['instance']['consume']}} wk
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have a more evocative name than wk? I haven't come up with an expansion that means anything to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

well-known is the intended expansion

I'll mull it over...


const { Fail } = assert;

const contractName = 'swaparoo';

/** @template SF @typedef {import('@agoric/zoe/src/zoeService/utils').StartResult<SF>} StartResult<SF> */
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion:

Suggested change
/** @template SF @typedef {import('@agoric/zoe/src/zoeService/utils').StartResult<SF>} StartResult<SF> */
/**
* @template SF
* @typedef {import('@agoric/zoe/src/zoeService/utils').StartResult<SF>} StartResult<SF>
*/

@dckc dckc force-pushed the dc-e2e-testing branch 4 times, most recently from 9bc9eb9 to 8452d0e Compare March 12, 2024 23:26
Base automatically changed from dc-e2e-testing to main March 16, 2024 17:34
dckc added 2 commits March 18, 2024 00:48
 - fix: get namesByAddressAdmin from privateArgs
 - refactor: subordinate electorate param

while writing docs, it became clear that Fee is more interesting
dckc added 6 commits March 18, 2024 00:48
 - feat: startMyCommittee
 - platform-goals/types: governance, vats, Zoe, ERTP types
   make them available ambiently
 - avoid linking to other packages
 - start committee
 - test-deploy-tools: test local work-alikes against sdk
  - harden
  - supply puppet electionManager
 - puppet-gov: test support
 - test-swap-wallet: restore to working order with gov
 - test-vote-fee-change: start governed; change Fee
 - test-vote-by-committee: vote by committee members
   with snapshots of offer details
 - initialize chainTimerService to modern time
@dckc dckc merged commit a4d767e into main Mar 18, 2024
1 check passed
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.

I got stumped trying to build out the UI for this, left a couple comments related

const {
[committeeName]: { voterAddresses },
} = config?.options || {};
assert(voterAddresses, 'no voter addresses???');
Copy link
Contributor

Choose a reason for hiding this comment

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

This assert always passes as long as voteAddresses is an empty object {}. Should we assert that it contains some values?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, yes, we should... but I wouldn't be surprised if some tests would need work to meet that constraint.

const depositFacet = E(namesByAddress).lookup(addr, 'depositFacet');
await E.when(invitationP, invitation =>
E(depositFacet).receive(invitation),
).catch(err => console.error(`failed deposit to ${debugName}`, err));
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried adding an address through the env variable SWAP_GOV_ADDR in the Makefile which seemed to successfully pipe through to this code, but I'm seeing an error here when deploying the contract:

"nameKey" not found: "agoric1rwwley550k9mmk6uq6mm6z4udrg8kyuyvfszjk"

This is confusing because in the logs I can see that this address has a smart wallet and is opening vaults before it gets to this point. Is there something wrong with namesByAddress in this context?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe you're running on a chain that doesn't have the fix for this one yet? I don't recall what release it's in.

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.

3 participants