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

Wallet offer fees #3678

Merged
merged 4 commits into from
Aug 14, 2021
Merged

Wallet offer fees #3678

merged 4 commits into from
Aug 14, 2021

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Aug 13, 2021

Closes #3650 and Closes #3669

UI changes are especially for @dtribble.

Wallet changes are especially for @katelynsills.

The test changes are for anybody. (A lot needed to change because of the automatic addition of the 'RUN' issuer during wallet initialization, which bumped up the sparse int generator in the board).

@michaelfig michaelfig self-assigned this Aug 13, 2021
@michaelfig
Copy link
Member Author

@katelynsills I wired in the new makeZoeKit, but starting a solo or chain still results in:

Error#1: A feePurse must be provided, not undefined
Error: A feePurse must be provided, not (an undefined)
 at makeError ()
 at fail ()
 at baseAssert ()
 at assertFeePurse ()
 at ()
 at ()

Is there any chance that you could make a missing feePurse only an error if the charged fee amount is nonempty? That would enable us to move forward, but there's still a lot of work to do to have the chain bootstrap somehow bypass Zoe fees for the contracts it's instantiating, etc.

@katelynsills
Copy link
Contributor

katelynsills commented Aug 13, 2021

@katelynsills I wired in the new makeZoeKit, but starting a solo or chain still results in:

Error#1: A feePurse must be provided, not undefined
Error: A feePurse must be provided, not (an undefined)
 at makeError ()
 at fail ()
 at baseAssert ()
 at assertFeePurse ()
 at ()
 at ()

Is there any chance that you could make a missing feePurse only an error if the charged fee amount is nonempty? That would enable us to move forward, but there's still a lot of work to do to have the chain bootstrap somehow bypass Zoe fees for the contracts it's instantiating, etc.

Let's not try to test end to end yet. 3322-add-fee-zoe-methods doesn't charge any fees, just asserts that a fee purse is there, so I think it's premature to put in logic to bypass charging fees.

The WIP PR (#3674) on top of 3322-add-fee-zoe-methods does charge fees, and takes care of the chain bootstrap process. The way it's working is that Zoe still charges fees even for the Treasury, but bootstrap process gets a payment of RUN to use for distributing to users and for launching the treasury.

So I think there's not a lot of work to do for the bootstrap process, but the PRs aren't ready yet for end to end testing.

@michaelfig
Copy link
Member Author

So I think there's not a lot of work to do for the bootstrap process, but the PRs aren't ready yet for end to end testing.

Okay, glad to hear that. I just got a little scared. 😨

@katelynsills
Copy link
Contributor

I don't know if this would help with the testing, but if you separate the work #3650 from #3669, you could test #3650 against current master and get that approved and merged separately.

Base automatically changed from 3322-add-fee-zoe-methods to 3294-zoe-metering August 14, 2021 01:43
@michaelfig michaelfig force-pushed the mfig-3650-wallet-offer-fees branch from d8c2253 to 1dd0588 Compare August 14, 2021 02:02
@michaelfig
Copy link
Member Author

I don't know if this would help with the testing, but if you separate the work #3650 from #3669, you could test #3650 against current master and get that approved and merged separately.

The current master already has a way to see the invitationDetails (a little </> debug button), so until I'm testing against specific Zoe fees and expirations, we don't really gain anything from this change.

@katelynsills
Copy link
Contributor

The current master already has a way to see the invitationDetails (a little </> debug button), so until I'm testing against specific Zoe fees and expirations, we don't really gain anything from this change.

The invitation details for the invitation attached an offer to be approved or not approved? Ah, if so, I did not know that.

@michaelfig
Copy link
Member Author

The current master already has a way to see the invitationDetails (a little </> debug button), so until I'm testing against specific Zoe fees and expirations, we don't really gain anything from this change.

The invitation details for the invitation attached an offer to be approved or not approved? Ah, if so, I did not know that.

Oh, nevermind... I'm wrong. Once this PR lands we'll have the invitationDetails.

{#if expiry}
<div>
<h6>Expiry</h6>
{formatDateNow(parseFloat(expiry) * 1000)}
Copy link
Member

Choose a reason for hiding this comment

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

Why is the expiration a float? or rather, why does this need parseFloat?

Copy link
Member

Choose a reason for hiding this comment

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

Because it's in seconds.

return makeZoeKit(
adminVat,
shutdownZoeVat,
undefined,
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a change corresponding to adding a 4th argument.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed

@dtribble
Copy link
Member

OK it looks good but can't be approved until the rest of Kate's changes go in. I'll rereview then.

@katelynsills
Copy link
Contributor

OK it looks good but can't be approved until the rest of Kate's changes go in. I'll rereview then.

Oh, why is that? The base for this PR is 3294-zoe-metering, which is the base for the zoe metering changes and includes all the approved changes so far. If we approve and merge, it won't go into master, it will go into the Zoe metering changes.

@michaelfig
Copy link
Member Author

OK it looks good but can't be approved until the rest of Kate's changes go in. I'll rereview then.

Oh, why is that? The base for this PR is 3294-zoe-metering, which is the base for the zoe metering changes and includes all the approved changes so far. If we approve and merge, it won't go into master, it will go into the Zoe metering changes.

Ah, true. I think this just needs your approval to merge too, @katelynsills, since you have ownership of that base branch.

Copy link
Contributor

@katelynsills katelynsills left a comment

Choose a reason for hiding this comment

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

LGTM!

@michaelfig michaelfig merged commit 0f151ca into 3294-zoe-metering Aug 14, 2021
@michaelfig michaelfig deleted the mfig-3650-wallet-offer-fees branch August 14, 2021 06:11
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.

3 participants