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

test: fusdc multichain #10618

Open
wants to merge 10 commits into
base: pc/fusdc-bech32-hooks
Choose a base branch
from
Open

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented Dec 4, 2024

closes: #10597

Description

Adds multichain-testing test for FUSDC happy path. Includes:

  • accepting oracle operator invitations
  • funding liquidity pool with bridged USDC
  • simulating advance request via mockCctpTxEvidence submissions
  • simulating CCTP mint and settlement flow
  • LP returns shares for more USDC than deposited

Ensures advance appears in EUD account, Liquidity Pool is repaid, and corresponding TxStatus updates are recorded in vstorage.

Security Considerations

None, test code.

Scaling Considerations

None really. For CI load, the test takes about 32 seconds after setup.

Documentation Considerations

None

Testing Considerations

PR only includes tests and test support.

Upgrade Considerations

None

Copy link

cloudflare-workers-and-pages bot commented Dec 4, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: e860814
Status: ✅  Deploy successful!
Preview URL: https://92b3e916.agoric-sdk.pages.dev
Branch Preview URL: https://pc-fusdc-multichain.agoric-sdk.pages.dev

View logs

@0xpatrickdev 0xpatrickdev added the force:integration Force integration tests to run on PR label Dec 4, 2024
@0xpatrickdev 0xpatrickdev changed the base branch from master to pc/fusdc-multichain-support December 5, 2024 23:24
Base automatically changed from pc/fusdc-multichain-support to master December 6, 2024 00:54
Copy link

github-actions bot commented Dec 6, 2024

Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label.

@0xpatrickdev 0xpatrickdev changed the base branch from master to pc/fusdc-vstorage-updates December 10, 2024 01:01
@0xpatrickdev 0xpatrickdev force-pushed the pc/fusdc-vstorage-updates branch from e3d94c3 to bf7b154 Compare December 10, 2024 18:11
@0xpatrickdev 0xpatrickdev force-pushed the pc/fusdc-vstorage-updates branch 2 times, most recently from 21d5049 to 287e5be Compare December 10, 2024 22:57
@0xpatrickdev 0xpatrickdev force-pushed the pc/fusdc-multichain branch 4 times, most recently from 1c5abba to ec38514 Compare December 11, 2024 00:06
@0xpatrickdev 0xpatrickdev marked this pull request as ready for review December 11, 2024 00:09
@0xpatrickdev 0xpatrickdev requested a review from a team as a code owner December 11, 2024 00:09
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.

I gotta run.

I rushed this review a little, so you may want more eyes. But as you say, only test code, so low risk.

harden(oracleMnemonics);

export const makeFeedPolicy = (nobleAgoricChannelId: IBCChannelID) => {
return JSON.stringify({
Copy link
Member

Choose a reason for hiding this comment

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

can the caller JSON.stringify it? It would be nice to @returns { FeedPolicy }.

await startContract(contractName, contractBuilder, {
oracle: keys(oracleMnemonics).map(n => `${n}:${wallets[n]}`),
usdcDenom: usdcDenom,
feedPolicy: makeFeedPolicy(nobleAgoricChannelId),
Copy link
Member

Choose a reason for hiding this comment

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

startContract wants a string for feedPolicy? ah... it's just taking flags(). Let's stringify here.

Comment on lines 18 to 22
Arbitrum: {
cctpTokenMessengerAddress: '0x19330d10D9Cc8751218eaf51E8885D058642E08A',
chainId: 42161,
confirmations: 2,
nobleContractAddress: '0x19330d10D9Cc8751218eaf51E8885D058642E08A',
Copy link
Member

Choose a reason for hiding this comment

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

are those arbitrary values chosen by us?

Or is there an external design constraint here? If so, let's please cite it.

Copy link
Contributor

Choose a reason for hiding this comment

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

For context: https://developers.circle.com/stablecoins/evm-smart-contracts

That's for cctpTokenMessengerAddress, but nobleContractAddress could be the same or different. From codecider:

nobleContractAddress: Hex;
- The address of Noble's wrapper contract that users interact with via NobleExpress
- msg.sender of DepositAndBurn must be this address to prevent users from calling replaceDepositForBurn

I'm not sure where exactly we'd find the noble wrapper contract addresses (if they exist). Might need to ask those folks directly.

Copy link
Member

Choose a reason for hiding this comment

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

yes, let's add that pointer, @samsiegart

I wonder how to track down the date the contract was deployed. A date is always a nice thing to have in a citation.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #10679. Feel free to provide suggestions for a better name in PR or offline

Copy link
Member

Choose a reason for hiding this comment

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

on 2nd thought, to answer my own question, yes, we did choose this address: this is a test config. If we want to re-deploy the contract at a different address in our test env, we don't have to coordinate with anybody else.

provisionSmartWallet,
startContract,
} = common;
deleteTestKeys(accounts).catch();
Copy link
Member

Choose a reason for hiding this comment

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

we want/need this to finish before setupTestKeys(...), right?

Suggested change
deleteTestKeys(accounts).catch();
await deleteTestKeys(accounts).catch();

we don't have lint for dangling promises in multichain-testing?

deleteTestKeys(accounts).catch();
const wallets = await setupTestKeys(accounts, values(oracleMnemonics));

// provision oracle wallets first so invitation deposits don't fail
Copy link
Member

Choose a reason for hiding this comment

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

I/we should check that product is OK with this constraint.

Copy link
Member

Choose a reason for hiding this comment

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

confirmed

export function makeRandomDigits(digits = 2n) {
if (digits < 1n) throw new Error('digits must be positive');
const maxValue = Math.pow(10, Number(digits)) - 1;
const num = Math.floor(Math.random() * (maxValue + 1));
Copy link
Member

Choose a reason for hiding this comment

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

Math.random is ambient authority. There's only 1 caller of this function. Please move it to the test script or inline it so that we're not exporting functions that close over ambient authority.

Copy link
Member Author

Choose a reason for hiding this comment

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

This seemed like too much to inline - I made randomNumber a parameter and put the Math.random() call in the test file.

invitationSpec: {
source: 'purse',
instance,
description: 'oracle operator invitation', // TODO export/import INVITATION_MAKERS_DESC
Copy link
Member

Choose a reason for hiding this comment

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

yeah; we should have a helper in @agoric/fast-usdc/tools/ for building this whole offer spec.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #10677

retryUntilCondition(
() => vstorageClient.queryData(`published.wallet.${addr}.current`),
({ offerToUsedInvitation }) => {
return offerToUsedInvitation[0][0] === `${name}-accept`;
Copy link
Member

Choose a reason for hiding this comment

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

can we be sure this is the 1st offer in that table?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this test, yes, because we are using fresh mnemonics

Comment on lines +192 to +222
// register forwarding address on noble
const txRes = nobleTools.registerForwardingAcct(
nobleAgoricChannelId,
recipientAddress,
);
Copy link
Member

Choose a reason for hiding this comment

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

hm. is this step in the diagrams in our current spec?
IOU a check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am a little hazy - but iirc the UI's backend service and/or OCW does this step

Copy link
Member

Choose a reason for hiding this comment

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

ah yes... it's there. arrow 4.

sequenceDiagram
    title Fast USDC Transaction Process
    participant NEA as Noble Express app<br/>[Browser]
    participant NC as Noble Chain<br/>[Noble]

rect rgb(240, 248, 255)
        NEA-->>NC: Register NFA
end
Loading


const mintAmount = 800_000n;

// TODO export CctpTxEvidence type
Copy link
Member

Choose a reason for hiding this comment

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

yikes!

This is a good test of the package, isn't it?

Copy link
Member Author

@0xpatrickdev 0xpatrickdev Dec 12, 2024

Choose a reason for hiding this comment

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

+1. I put exporting types as a req for: #10677

@0xpatrickdev 0xpatrickdev force-pushed the pc/fusdc-vstorage-updates branch from 287e5be to 475e720 Compare December 11, 2024 00:56
Base automatically changed from pc/fusdc-vstorage-updates to master December 11, 2024 01:43
Copy link

Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label.

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.

So cool to see this working!

Comment on lines 18 to 22
Arbitrum: {
cctpTokenMessengerAddress: '0x19330d10D9Cc8751218eaf51E8885D058642E08A',
chainId: 42161,
confirmations: 2,
nobleContractAddress: '0x19330d10D9Cc8751218eaf51E8885D058642E08A',
Copy link
Contributor

Choose a reason for hiding this comment

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

For context: https://developers.circle.com/stablecoins/evm-smart-contracts

That's for cctpTokenMessengerAddress, but nobleContractAddress could be the same or different. From codecider:

nobleContractAddress: Hex;
- The address of Noble's wrapper contract that users interact with via NobleExpress
- msg.sender of DepositAndBurn must be this address to prevent users from calling replaceDepositForBurn

I'm not sure where exactly we'd find the noble wrapper contract addresses (if they exist). Might need to ask those folks directly.

() => vstorageClient.queryData(`published.${contractName}.poolMetrics`),
({ shareWorth }) =>
!isGTE(currShareWorth.numerator, shareWorth.numerator),
'share worth numerator increases from deposit',
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this is reasonable for testing it worked. It would also be good to verify that the pool shares were received, but I think we can do that implicitly in the todo withdrawal test below.

Copy link
Member Author

Choose a reason for hiding this comment

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

doOffer checks that numWantsSatisfied is 1, so we can have confidence the pool shares were received.

No harm in explicitly checking in the test though, so I fixed up to include that. Also closed out the test.todo('lp withdraws')

@turadg turadg removed their request for review December 11, 2024 16:54
@0xpatrickdev 0xpatrickdev added automerge:rebase Automatically rebase updates, then merge and removed automerge:rebase Automatically rebase updates, then merge labels Dec 11, 2024
@0xpatrickdev 0xpatrickdev changed the base branch from master to pc/fusdc-bech32-hooks December 11, 2024 22:50
@0xpatrickdev 0xpatrickdev force-pushed the pc/fusdc-bech32-hooks branch 2 times, most recently from f081d9f to b9c275c Compare December 11, 2024 23:18
@0xpatrickdev 0xpatrickdev force-pushed the pc/fusdc-bech32-hooks branch 3 times, most recently from ba4fce5 to d369f08 Compare December 12, 2024 01:47
- rename `nobleContractAddress` to `attenuatedCttpBridgeAddress`
- include jsdoc comments for `ChainPolicy` type
- removed `chainType` from `ChainPolicy`. unused and should have been an enum. not needed for OCW currently
- include `chainInfo` and `assetInfo` as common builder options for orchestration contracts
- in the testing environment, we might see multiple USDC entries in `vbankAsset`. This change ensures the
  `byName` record contains the values needed for the test, reliant on the consistent ordering currently
  present in `vbankAsset` entries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FU multichain testing of happy path
4 participants