-
Notifications
You must be signed in to change notification settings - Fork 222
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
client support for Fast USDC #10735
client support for Fast USDC #10735
Conversation
Deploying agoric-sdk with
|
Latest commit: |
948d7b7
|
Status: | ✅ Deploy successful! |
Preview URL: | https://1c837511.agoric-sdk.pages.dev |
Branch Preview URL: | https://10677-fu-client-support.agoric-sdk.pages.dev |
c76dc1b
to
e49ad00
Compare
e49ad00
to
ea8de31
Compare
ea8de31
to
7374d68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops... I think I misinterpreted a request-for-review notification
So these comments might be a bit premature...
@@ -55,6 +55,11 @@ export interface TransactionRecord extends CopyRecord { | |||
status: TxStatus; | |||
} | |||
|
|||
export interface ContractRecord extends CopyRecord { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name... does it refer to "the record you'll get from vstorage if you query the contract's node"?
docstring, please?
: T extends 'agoricNames.instance' | ||
? Array<[string, Instance]> | ||
: T extends 'agoricNames.brand' | ||
? Array<[string, Brand]> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of the relevant code expects not just a Brand
but a Brand<'nat'>
or Brand<'set'>
.
const { USDC, FastLP } = await agoricNamesQ(vstorageClient).brands('nat'); |
const { Invitation } = await agoricNamesQ(vsc).brands('set'); |
I suppose one can add as Brand<'nat'>
, but it almost defeats the point. It's also helpful to reassemble the entries into a record. Do you want to add helpers for this sort of thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed this is low value because you'll often want the kind of brand. It's still true tho so I think it's worth having here.
: T extends 'fastUsdc' | ||
? ContractRecord | ||
: T extends 'fastUsdc.poolMetrics' | ||
? PoolMetrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still feels strange for client-utils to depend on fast-usdc. Should it depend on dapp-offer-up and dapp-agoric-basics likewise?
It seems like fast-usdc should extend the type from client-utils.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is weird but ok for now. We'll have to solve it for #10811
/** @type {import('../types.js').PoolMetrics} */ | ||
// @ts-expect-error it treats this as "unknown" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to get rid of the type annotation while you're at it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah thanks
settlementAddress: ChainAddressShape, | ||
intermediateRecipient: M.opt(ChainAddressShape), | ||
settlementAddress: M.opt(ChainAddressShape), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
What prompted it, I wonder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember but I'm guessing some type feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From description:
Everything in #10677 except the offer makers. I think that'll come through Noble integration.
I think you added the offer makers in packages/fast-usdc/src/clientSupport.js
right? Also, by Noble integration do you mean Noble Express UI? Because that won't be submitting any offers AFAIK. Only LPs and OCW will.
I'm assuming we're going to change the CLI and multichain-testing to use these offer functions in a later PR?
The actual code changes in this PR LGTM, so approving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to also add functions for accepting the operator invitation and submitting evidence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test for WithdrawFees
?
@@ -15,7 +15,7 @@ import { parseArgs } from 'node:util'; | |||
/** | |||
* @import {CoreEvalBuilder, DeployScriptFunction} from '@agoric/deploy-script-support/src/externalTypes.js' | |||
* @import {ParseArgsConfig} from 'node:util' | |||
* @import {FastUSDCConfig, FeedPolicy} from '@agoric/fast-usdc/src/types.js' | |||
* @import {FastUSDCConfig, FeedPolicy} from '@agoric/fast-usdc'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: there's a semicolon here but not for the lines above. I wonder if eslint/prettier should have a rule for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think ESlint has a linter for it. There's a Prettier plugin for JSDoc but it's quite buggy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's factor the offer makers out of existing code.
And as a result, let's leave BoardRemote
behind and go from Number
to ERTP ratio early.
* @param {number} opts.amount | ||
* @returns {import('@agoric/smart-wallet/src/offers.js').OfferSpec} | ||
*/ | ||
const makeDepositOffer = ({ brand }, instance, { offerId, amount }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
want
is required, and the keyword is USDC
, not Deposit
.
I'd like us to re-use the existing fast usdc deposit type as in:
agoric-sdk/packages/fast-usdc/src/cli/lp-commands.js
Lines 104 to 112 in f0ccde6
/** @type {USDCProposalShapes['deposit']} */ | |
const proposal = { | |
give: { | |
USDC: usdcAmount, | |
}, | |
want: { | |
PoolShare: fastLPAmount, | |
}, | |
}; |
We could calculate the want for them if they pass in metrics
or shareWorth
as in...
agoric-sdk/packages/fast-usdc/src/cli/lp-commands.js
Lines 101 to 102 in f0ccde6
const metrics = await swk.readPublished('fastUsdc.poolMetrics'); | |
const fastLPAmount = floorDivideBy(usdcAmount, metrics.shareWorth); |
We might want a slippage
parameter.
|
||
/** | ||
* @param {Pick< | ||
* import('@agoric/vats/tools/board-utils.js').AgoricNamesRemotes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's please stop using BoardRemote
and hence AgoricNamesRemotes
. Just use the ordinary types like Brand
.
ref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree eventually but can you accept that as being out of scope? The inter-protocol
clientSupport uses AgoricNamesRemotes
and I think if we're going to change it we should do both. #10491 looks like a good issue to schedule that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we're going to change it we should do both.
I prefer the "platform is fixed" idea where fast-usdc is independent of inter-protocol. But I can live with this.
|
||
// Reusable amount scaling similar to inter-protocol's approach | ||
const COSMOS_UNIT = 1_000_000n; | ||
const scaleDecimals = num => BigInt(num * Number(COSMOS_UNIT)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Number
has weird edge cases. For example, if the product inside BigInt
isn't an integer, it throws.
Let's stick to ERTP Amount
and Ratio
as in:
agoric-sdk/packages/fast-usdc/src/cli/lp-commands.js
Lines 42 to 46 in f0ccde6
const parseUSDCAmount = (amountString, usdc) => { | |
const USDC_DECIMALS = 6; | |
const unit = AmountMath.make(usdc, 10n ** BigInt(USDC_DECIMALS)); | |
return multiplyBy(unit, parseRatio(amountString, usdc)); | |
}; |
parseRatio
will take a Number
or a string
(anything that passes assertParsableNumber
), so we could relax amountString
likewise.
For commander-style arg parsing, we could export this one:
agoric-sdk/packages/fast-usdc/src/cli/lp-commands.js
Lines 28 to 36 in f0ccde6
const parseDecimal = arg => { | |
try { | |
assertParsableNumber(arg); | |
const n = Number(arg); | |
return n; | |
} catch { | |
throw new InvalidArgumentError('Not a number'); | |
} | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the thing clientSupport helpers do is provide the brand. I think what you're asking for is a bigger change. Again I'm not fully opposed but I think it should be separated from this PR and cover the inter-protocol helper too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I compromised by making it take a bigint but not the brand.
}); | ||
|
||
/** @satisfies {Record<string, import('@agoric/smart-wallet/src/types.js').OfferMaker>} */ | ||
export const Offers = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any particular reason to export this Offers
structure rather than the functions makeDepositOffer
and such?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have it handy but there was a PR for refactoring our tests to use these and it let you merge multiple of these into an Offers
object with all the makers you needed.
Also this provides a way to enforce that they all satisfy OfferMaker
.
* @param {string} opts.offerId | ||
* @returns {import('@agoric/smart-wallet/src/offers.js').OfferSpec} | ||
*/ | ||
const makeWithdrawFeesOffer = (instance, { offerId }) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is much less valuable; only a core-eval can withdraw fees.
But if we want to provide a helper, let's factor it out of...
export const distributeFees = async (permittedPowers, config) => { |
Note support for both absolute amounts and proportions.
f4f6a52
to
6e5d568
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, this is forward progress.
|
||
/** | ||
* @param {Pick< | ||
* import('@agoric/vats/tools/board-utils.js').AgoricNamesRemotes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we're going to change it we should do both.
I prefer the "platform is fixed" idea where fast-usdc is independent of inter-protocol. But I can live with this.
fastUsdc: { | ||
Deposit: makeDepositOffer, | ||
Withdraw: makeWithdrawOffer, | ||
BecomeOperator: makeOperatorOffer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see a WithdrawFees
offer maker factored our of distribute-fees.core.js
. But I suppose it's reasonable to treat core eval stuff differently.
05e5caf
to
fa56775
Compare
Fixes this warning from typdoc, ``` ./packages/SwingSet/src/controller/controller.js:480:21 - [warning] The relative path ../docs/run-policy.md is not a file and will not be copied to the output directory 480 * [`runPolicy`](../docs/run-policy.md) to rate-limit cleanups. ```
fa56775
to
948d7b7
Compare
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks: You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
closes: #10677
Description
Everything in #10677 including some offer makers. There are more opportunities but this lays the groundwork for those to be added as valuable.
Security Considerations
none
Scaling Considerations
none
Documentation Considerations
none
Testing Considerations
nothing special
Upgrade Considerations
not yet released