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: contract code to mint tickets #8

Merged
merged 26 commits into from
Mar 6, 2024
Merged

feat: contract code to mint tickets #8

merged 26 commits into from
Mar 6, 2024

Conversation

LuqiPan
Copy link
Collaborator

@LuqiPan LuqiPan commented Feb 9, 2024

My first stab at updating the contract to mint tickets, notably, update contract terms to accept inventory then check want against inventory to make sure we have enough inventory and check give against total price to make sure user offers enough tokens.

This is still pretty rough so any and all feedbacks are welcomed.

TODOs:

  • make the last test case in test-contract.js pass, it's using mockBootstrap and I don't know quite understand how it works
  • update comments and JSDocs accordingly

@LuqiPan LuqiPan changed the title 939 mint tickets feat: contract code to mint tickets Feb 9, 2024
Copy link
Collaborator Author

@LuqiPan LuqiPan left a comment

Choose a reason for hiding this comment

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

Posting my open questions with current code

contract/src/agoric-basics.contract.js Outdated Show resolved Hide resolved
contract/src/agoric-basics.contract.js Outdated Show resolved Hide resolved
* @param {number} n
* @returns {Amount}
*/
const multiply = (amount, n) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

question

I didn't see a AmountMath.multiply function so I implemented it myself

Copy link
Member

Choose a reason for hiding this comment

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

import {
  makeRatio,
  multiplyBy,
} from '@agoric/zoe/src/contractSupport/ratio.js';
import { Nat } from '@endo/nat';

/**
 *
 * @param {Amount<'nat'>} amount
 * @param {number} n
 * @returns {Amount}
 */
const multiply = (amount, n) => {
  const r = makeRatio(Nat(n), amount.brand, 1n);
  return multiplyBy(amount, r);
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I seem to have some trouble making multiply work for Amount<AssetKind> instead of Amount<'nat'>, am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

Our ratio library doesn't support non-fungible amounts and says so in its type annotations... which means you may have to trace thru all the callers and update their types to Amount<'nat'>.

In some places, a typecast is called for. For example, zcf.getTerms() doesn't know whether the brands are for fungible assets or not. And you're not in a position to doubt the caller that started the contract - that's part of the reliance set. You could ask the brand what its AssetKind is, but it could lie, so it's not worth the bother.

contract/src/agoric-basics.contract.js Outdated Show resolved Hide resolved
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.

thanks for sharing in draft form. good stuff here.

contract/src/agoric-basics.contract.js Outdated Show resolved Hide resolved
contract/src/agoric-basics.contract.js Outdated Show resolved Hide resolved
contract/src/agoric-basics.contract.js Outdated Show resolved Hide resolved
contract/src/agoric-basics.contract.js Outdated Show resolved Hide resolved
* @param {number} n
* @returns {Amount}
*/
const multiply = (amount, n) => {
Copy link
Member

Choose a reason for hiding this comment

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

import {
  makeRatio,
  multiplyBy,
} from '@agoric/zoe/src/contractSupport/ratio.js';
import { Nat } from '@endo/nat';

/**
 *
 * @param {Amount<'nat'>} amount
 * @param {number} n
 * @returns {Amount}
 */
const multiply = (amount, n) => {
  const r = makeRatio(Nat(n), amount.brand, 1n);
  return multiplyBy(amount, r);
};

contract/src/agoric-basics.contract.js Outdated Show resolved Hide resolved
contract/test/test-contract.js Outdated Show resolved Hide resolved
contract/test/test-contract.js Outdated Show resolved Hide resolved
contract/src/agoric-basics-proposal.js Show resolved Hide resolved
contract/src/agoric-basics.contract.js Outdated Show resolved Hide resolved
@dckc
Copy link
Member

dckc commented Feb 9, 2024

update UI code and tests accordingly

updating the UI code seems to fit more comfortably in a separate PR

contract/src/agoric-basics.contract.js Outdated Show resolved Hide resolved
* @param {number} n
* @returns {Amount}
*/
const multiply = (amount, n) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I seem to have some trouble making multiply work for Amount<AssetKind> instead of Amount<'nat'>, am I missing something?

[newItems, buyerSeat, want],
[buyerSeat, proceeds, { Price: totalPrice }],
// tickets from inventory to buyer
[inventorySeat, buyerSeat, want],
]),
);

buyerSeat.exit(true);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

question, for my learning

What to do with proceeds and inventorySeat?

Seems like we could get the proceeds when contract is closed by the creator via creatorFacet. Should we keep inventorySeat open even in the case when all tickets are sold?

Copy link
Member

Choose a reason for hiding this comment

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

yes, a creatorFacet method is a typical way to collect the proceeds. (Seems fine to postpone to a later PR, if we bother to do it at all)
See, for example, collectFees.js and the contracts that import it.

as to inventorySeat... when they're all sold, we could shut the contract down (there's a shutdown method on zcf)

Or we could have a creatorFacet method to resupply it for the next event or something. But that seems like over-kill.

Comment on lines 100 to 102
// M.splitRecord({ tradePrice: AmountShape, maxTickets: M.nat() }),
// getting an error of
// customTerms: inventory: [1]: {} - Must have missing properties ["tradePrice","maxTickets"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

question

Not sure why M.splitRecord(...) doesn't work here- inventory has tradePrice and maxTickets in the values

Copy link
Member

Choose a reason for hiding this comment

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

I took the liberty of pushing a fix

@dckc dckc mentioned this pull request Mar 4, 2024
3 tasks
@LuqiPan LuqiPan marked this pull request as ready for review March 5, 2024 17:37
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!

@LuqiPan LuqiPan merged commit 0fb9ecc into main Mar 6, 2024
1 check passed
@LuqiPan LuqiPan deleted the 939-mint-tickets branch March 6, 2024 04:57
@LuqiPan LuqiPan restored the 939-mint-tickets branch March 6, 2024 04:57
@LuqiPan LuqiPan mentioned this pull request Mar 6, 2024
@aj-agoric
Copy link

Marking this PR as done in the project board since this is merged.

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