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

chore: fusdc chain policy limits #10814

Merged
merged 3 commits into from
Jan 11, 2025
Merged

chore: fusdc chain policy limits #10814

merged 3 commits into from
Jan 11, 2025

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented Jan 8, 2025

closes: #10812

Description

  1. Adds rateLimits to ChainPolicy:
rateLimits: {
  /** do not advance more than this amount for an individual transaction */
  tx: bigint;
  /** do not advance more than this amount per block window */
  blockWindow: bigint;
  /** the number of blocks to consider for `blockWindow` */
  blockWindowSize: number;
};
  1. Renames attenuatedCttpBridgeAddress to attenuatedCttpBridgeAddresses to accept a list of attenuated bridge contracts.
  2. Adds a fast-usdc-tool.ts script to multichain-testing to bootstrap fast-usdc and set up an environment for testing with the OCW.

Security Considerations

This is an additional line of defense for FUSDC to limit the total value flowing through the system over a window of blocks.

Scaling Considerations

None, handled in off-chain oracle operator software.

Documentation Considerations

None

Testing Considerations

Updates existing tests

Upgrade Considerations

None, targeting initial release of FUSDC.

@0xpatrickdev 0xpatrickdev requested a review from a team as a code owner January 8, 2025 00:02
Copy link

cloudflare-workers-and-pages bot commented Jan 8, 2025

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1b1b52d
Status: ✅  Deploy successful!
Preview URL: https://b65725c0.agoric-sdk.pages.dev
Branch Preview URL: https://pc-chain-policy.agoric-sdk.pages.dev

View logs

@0xpatrickdev 0xpatrickdev requested a review from dckc January 8, 2025 00:13
@0xpatrickdev 0xpatrickdev added the force:integration Force integration tests to run on PR label Jan 8, 2025
@0xpatrickdev 0xpatrickdev marked this pull request as draft January 8, 2025 23:40
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.

oops; I gotta run.
this is what I have so far...


if (values.help) {
console.log(`
Usage:
Copy link
Member

Choose a reason for hiding this comment

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

please move this Usage string constant to the top of the file.

I was going to ask for a file comment, but this is in some ways better.

The file name gave very little clue about what it's for too... though now that I see what it's for, I suppose that name is not bad. But maybe fast-usdc-tool.ts would be better.

./fast-usdc.ts fund-pool
./fast-usdc.ts fund-faucet
`);
process.exit(0);
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather we don't hide use of this very powerful process.exit authority under the name parseCommandLine(). Return undefined or throw or something instead, please?

};

const main = async () => {
const { command, mnemonic, oracles, provisionOracles } = parseCommandLine();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const { command, mnemonic, oracles, provisionOracles } = parseCommandLine();
const job = parseCommandLine();
if (!job) return;
const { command, mnemonic, oracles, provisionOracles } = job;

);
}
},
} as unknown as ExecutionContext;
Copy link
Member

Choose a reason for hiding this comment

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

would this suffice?

Suggested change
} as unknown as ExecutionContext;
} as Pick<ExecutionContext, 'log' | 'is'>;

const assertProvisioned = async address => {
try {
await vstorageClient.queryData(`published.wallet.${address}.current`);
return true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return true;
return;

},
nobleAgoricChannelId: 'channel-21',
nobleDomainId: 4,
body: '#{"chainPolicies":{"Arbitrum":{"attenuatedCttpBridgeAddress":"0xe298b93ffB5eA1FB628e0C0D55A43aeaC268e347","blockWindowSize":10,"cctpTokenMessengerAddress":"0x19330d10D9Cc8751218eaf51E8885D058642E08A","chainId":42161,"confirmations":2,"maxAmountPerBlockWindow":"+20000000000"}},"nobleAgoricChannelId":"channel-21","nobleDomainId":4}',
Copy link
Member

Choose a reason for hiding this comment

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

the function for documenting vstorage supports rendering this nicely. There's a show function or something.

return JSON.parse(feedPolicy);
const parsed = JSON.parse(feedPolicy);
if (!parsed.chainPolicies) {
// chainPolicies contain bigints which can't be stringified
Copy link
Member

Choose a reason for hiding this comment

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

this should use LegibleCapData then, I think

Copy link
Member

Choose a reason for hiding this comment

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

oic... this is saying "if the CLI arg doesn't have chainPolicies, fill it in from MAINNET"
maybe that's good enough

*/
const publishFeedPolicy = async (node, policy) => {
const publishFeedPolicy = async (node, marshaller, policy) => {
Copy link
Member

Choose a reason for hiding this comment

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

can policy have remotables in it? if not, you can use a data-only marshaller that's local to this function.

@@ -32,7 +35,7 @@ const publishFeedPolicy = async (node, policy) => {
* @param {{ options: LegibleCapData<{feedPolicy: FeedPolicy & Passable}> }} config
*/
export const updateFastUsdcPolicy = async (
{ consume: { agoricNames, chainStorage } },
{ consume: { agoricNames, board, chainStorage } },
Copy link
Member

Choose a reason for hiding this comment

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

The board marshaller looks like overkill.

*/
const publishFeedPolicy = async (node, policy) => {
const publishFeedPolicy = async (node, marshaller, policy) => {
Copy link
Member

Choose a reason for hiding this comment

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

again, I don't think the marshaller needs to be passed in

@0xpatrickdev 0xpatrickdev force-pushed the pc/chain-policy branch 2 times, most recently from 6555348 to 378f571 Compare January 9, 2025 01:05
cctpTokenMessengerAddress: '0x1682Ae6375C4E4A97e4B583BC394c861A46D8962',
chainId: 8453,
confirmations: 2,
maxAmountPerBlockWindow: 20000000000n,
Copy link
Member

Choose a reason for hiding this comment

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

to get this to show up next to blockWindowSize, maybe...

Suggested change
maxAmountPerBlockWindow: 20000000000n,
blockWindowMaxAmount: 20000000000n,

};

const start = async () => {
if (!chainInfo.noble) {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the env stuff is gone (yay!), but I still can't see how chainInfo.noble is anything but a design-time constant.

@0xpatrickdev 0xpatrickdev force-pushed the pc/chain-policy branch 2 times, most recently from 7088f49 to 3ec61df Compare January 10, 2025 22:47
@0xpatrickdev 0xpatrickdev marked this pull request as ready for review January 10, 2025 22:47
Copy link
Member Author

@0xpatrickdev 0xpatrickdev 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 the feedback @dckc . I think all comments are addressed, thanks for spending time to walk me through the data only marshaller.

@0xpatrickdev 0xpatrickdev requested a review from dckc January 10, 2025 22:48
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.

👍

utility script to assist with local development. includes:
- start contract with optional --oracles
- fund agoric faucet with USDC from remote chain
- fund Liquidity Pool for USDC
- provision smart wallet (using --mnemonic)
- ensure `ChainPolicy` can support multiple bridge attenuation contracts
@0xpatrickdev 0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Jan 10, 2025
@mergify mergify bot merged commit b58d523 into master Jan 11, 2025
87 of 96 checks passed
@mergify mergify bot deleted the pc/chain-policy branch January 11, 2025 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

limits and tiered confirmations in ChainPolicy
2 participants