-
Notifications
You must be signed in to change notification settings - Fork 206
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
chore(acceptance): add tests for PSM to z:acceptance
#10273
base: master
Are you sure you want to change the base?
Conversation
b167d7d
to
0c1ae50
Compare
Deploying agoric-sdk with Cloudflare Pages
|
This comment was marked as resolved.
This comment was marked as resolved.
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 did a code quality pass. I don't have PSM swapped in enough rn to evaluate whether this fully closes the issue.
|
||
// @ts-expect-error initialization | ||
const balance = await getBalances([userAddress], expectedAnchorFunds.denom); | ||
t.is(balance >= BigInt(expectedAnchorFunds.value), true); |
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 also doesn't have a diagnostic message. I think a simple assert
would be more appropriate here, so you don't have to thread the t
(especially since the any
erases the type)
t.is(balance >= BigInt(expectedAnchorFunds.value), true); | |
assert(balance >= BigInt(expectedAnchorFunds.value)); |
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.
resolved in 712f970
const giveAnchor = (base, fee) => { | ||
return Math.ceil(base / (1 - fee)); | ||
}; |
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.
consider more concise for these math expressions:
const giveAnchor = (base, fee) => { | |
return Math.ceil(base / (1 - fee)); | |
}; | |
const giveAnchor = (base, fee) => Math.ceil(base / (1 - fee)); |
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.
resolved in 712f970
* @param {any} t | ||
* @param {any} metricsBefore | ||
* @param {any} balancesBefore |
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.
please use real types
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.
resolved in 712f970
@@ -9,6 +9,7 @@ | |||
"type": "module", | |||
"license": "Apache-2.0", | |||
"dependencies": { | |||
"@agoric/ertp": "0.16.3-u17.1", |
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 doesn't match the @agoric/internal
version so there are duplicates.
Please make them both"dev"
so they're testing the latest master push. The version won't change unless the yarn.lock
changes, which can't happen without a PR, and that would have to pass CI to merge.
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.
made both of them dev
and re-enforced yarn.lock
by doing;
cd proposals/z:acceptance
rm yarn.lock
touch yarn.lock
yarn install
addressed in 712f970
} from '@agoric/synthetic-chain'; | ||
import { getBalances } from './test-lib/utils.js'; | ||
|
||
// Export these from synthetic-chain? |
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.
perhaps eventually. But having them in ENV gives us flexibility for other runtimes using them
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.
Why not we do them both? We do the same for ATOM_DENOM
=>
https://github.com/Agoric/agoric-3-proposals/blob/93bb953db209433499db08ae563942d1bf7eeb46/packages/synthetic-chain/src/lib/constants.js#L11
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.
sure. Just if they're in ENV that's the source of truth and any module can be made to export them
{ committeeAddrs: [GOV1ADDR, GOV2ADDR, GOV3ADDR], position: 0 }, | ||
); | ||
|
||
// Replace when https://github.com/Agoric/agoric-sdk/pull/10171 is in |
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.
please land that first cc @Chris-Hibbert
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.
@Chris-Hibbert approved it yesterday => #10171
So, as I understand, we should update this one with whatever is landed in #10171 then merge into master?
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.
Merge #10171 to master then rebase this onto master that has it.
0c1ae50
to
324d3ee
Compare
Refs: Agoric/BytePitchPartnerEng#23 fix(acceptance-psm): address change requests
324d3ee
to
712f970
Compare
Addressed all change requests outside of #10273 (comment). Should I ask for a re-review once the last one is done as well? @turadg |
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.
Code LGMT. I haven't reviewed for whether this covers all the PSM testing requirements.
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 meant to separate the critical comments from the suggestions, but it's taking too long.
Consider these all suggestions. I'll do another pass to look for anything critical.
import { getBalances } from './test-lib/utils.js'; | ||
|
||
// Export these from synthetic-chain? | ||
export const USDC_DENOM = process.env.USDC_DENOM; |
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.
Why export
? Importing from a .test.js
file would mean running the tests. And I think even that fails with some sort of "ava test only work with the ava runner command" diagnostic.
newUser: { | ||
name: 'new-psm-trader', | ||
fund: { | ||
denom: USDC_DENOM ? USDC_DENOM : 'fake', |
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 use / test the fake
case? I suspect we rely on USDC_DENOM
being NonNullish
. Better to do that above.
import { getBalances } from './test-lib/utils.js'; | ||
|
||
// Export these from synthetic-chain? | ||
export const USDC_DENOM = process.env.USDC_DENOM; |
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.
re fake
below...
export const USDC_DENOM = process.env.USDC_DENOM; | |
export const USDC_DENOM = NonNullish(process.env.USDC_DENOM); |
otherUser: { | ||
name: 'gov1', | ||
fund: { | ||
denom: USDC_DENOM ? USDC_DENOM : 'fake', |
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.
likewise
* @param {string} wanted | ||
* @param {string} [from] | ||
*/ | ||
export const bankSend = (addr, wanted, from = VALIDATORADDR) => { |
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.
suggestion; feel free to ignore
bankSend
is using ambient authority to access the keychain and filesystem. Please have the caller use makeAgd
and have bankSend
accept an Agd
argument.
import { boardSlottingMarshaller, makeFromBoard } from './rpc.js'; | ||
import { | ||
addUser, | ||
agd, |
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.
suggestion; feel free to ignore
That form of agd
uses ambient authority. Please avoid it. (bonus points for a PR to deprecate it)
offerArgs: { | ||
instance: instances[instanceName], | ||
params, | ||
deadline: BigInt(deadline * 60 + Math.round(Date.now() / 1000)), |
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.
suggestion; feel free to ignore
Date.now()
is ambient authority. Please pass it in explicitly.
instanceName, | ||
newParams, | ||
deadline, | ||
offerId = Date.now().toString(), |
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.
offerId = Date.now().toString(), | |
nowVal = Date.now(), | |
offerId = nowVal.toString(), |
offerArgs: { | ||
instance: instances[instanceName], | ||
params, | ||
deadline: BigInt(deadline * 60 + Math.round(Date.now() / 1000)), |
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.
deadline: BigInt(deadline * 60 + Math.round(Date.now() / 1000)), | |
deadline: BigInt(deadline * 60 + Math.round(nowVal / 1000)), |
deadline, | ||
offerId = Date.now().toString(), | ||
}) => { | ||
const charterAcceptOfferId = await agops.ec( |
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.
suggestion; feel free to ignore
agops
uses ambient authority. synthetic-chain doesn't have a handy alternative, but in dapp-orchestration-basics, one technique I recently used is for the caller to do:
const execFileP = promisify(childProcess.execFile);
const npx = (file: string, args: string[]) =>
execFileP('npx', ['--no-install', file, ...args]);
https://github.com/Agoric/dapp-orchestration-basics/blob/main/e2e-testing/scripts/deploy-cli.ts
and then here we would do
const charterAcceptOfferId = await agops.ec( | |
const charterAcceptOfferId = await npx('agops' , ['ec, |
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 tests cover the 6 requested items (New user can complete a swap, ...).
This approval presumes you'll address any lint failures, as usual. No need to re-request review once you've done that.
I found it a little challenging to navigate the split between psm.test.js
and psm-lib.js
in some places. I made some suggestions about factoring. Those are suggestions; feel free to act on them, respond to them, or ignore them, at your discretion.
export const USDC_DENOM = process.env.USDC_DENOM | ||
? process.env.USDC_DENOM | ||
: 'no-denom'; |
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 the 'no-denom'
path exercised / tested? How about just throwing?
export const USDC_DENOM = process.env.USDC_DENOM | |
? process.env.USDC_DENOM | |
: 'no-denom'; | |
export const USDC_DENOM = NonNullish(process.env.USDC_DENOM); |
export const USDC_DENOM = process.env.USDC_DENOM | ||
? process.env.USDC_DENOM | ||
: 'no-denom'; | ||
export const PSM_PAIR = process.env.PSM_PAIR?.replace('.', '-'); |
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.
.replace()
will fail if PSM_PAIR
is undefined, right?
export const PSM_PAIR = process.env.PSM_PAIR?.replace('.', '-'); | |
export const PSM_PAIR = NonNullish(process.env.PSM_PAIR).replace('.', '-'); |
giveMintedFeeVal: 10n, // in % | ||
wantMintedFeeVal: 10n, // in % | ||
mintLimit: 500n * 1_000_000n, // in IST | ||
deadline: 1, // in minutes |
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 this really a deadline, i.e. an absolute time? or is it a duration?
await initializeNewUser(name, fund); | ||
// Replace when https://github.com/Agoric/agoric-sdk/pull/10171 is in | ||
await waitForBlock(3); | ||
|
||
await checkUserInitializedSuccessfully(name, fund); |
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.
How about folding checkUserInitializedSuccessfully
into initializeNewUser
?
|
||
const balancesAdjusted = []; | ||
|
||
balances.forEach(({ denom, 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.
as the linter says...
Prefer for...of instead of Array.forEach
await agopsPsm(psmTrader, [ | ||
'swap', | ||
'--pair', | ||
process.env.PSM_PAIR, |
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.
why not use the PSM_PAIR
from above? (line 42)
'--feePct', | ||
wantMintedFeeVal, | ||
]); | ||
await waitForBlock(5); |
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 prefer that the agopsPsm
function takes care of any relevant synchronization.
Is that straightforward?
Also: is agopsPsm
ever called with anything other than swap
? If not, how about narrowing it to psmSwap(...)
?
// Replace when https://github.com/Agoric/agoric-sdk/pull/10171 is in | ||
await waitForBlock(3); |
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.
#10171 is in. now is the time to replace this, yes?
await waitForBlock(5); | ||
|
||
// Now check if failed with correct error message | ||
await checkSwapExceedMintLimit(t, otherAddr, metricsBefore); |
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.
Pushing the t.is(...)
etc. down into checkSwapExceedMintLimit
makes this code somewhat hard to follow.
What do you think of:
await checkSwapExceedMintLimit(t, otherAddr, metricsBefore); | |
const walletUpdate = await getWalletUpdate(otherAddr, 'offerStatus'); | |
t.is(walletUpdate.status.error, 'Error: Request would exceed mint limit'); | |
t.like(await getPsmMetrics(anchor), { mintedPoolBalance: metricsBefore.mintedPoolBalance }); |
where getWalletUpdate
throws if offerResult.updated !=
the 2nd arg.
Library functions that read data are straightforward. For non-read operations, making a swap is a clearly scoped thing to put in a helper function. But functions that mix reading data and doing assertions with t
are hard to follow.
* | ||
* @param {number} base | ||
* @param {number} fee | ||
* @returns |
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 suspect this makes yarn lint
fail, so it needs to be cleaned up before the PR is green.
closes: Agoric/BytePitchPartnerEng#23
Description
Current version of
z:acceptance
tests do not cover PSM at all. The goal of this PR is to make sure we cover test cases in Agoric/BytePitchPartnerEng#23 prepared by @otoole-brendanSecurity Considerations
None.
Scaling Considerations
None.
Documentation Considerations
None.
Testing Considerations
The tests assume that
anchorPerMinted
ratio is 1. I think this is a safe assumption because;boot
tests for the PSM has the same assumptionUSDC_axl
instance which is something we shouldn't worry about anytime soon.Upgrade Considerations
None.