-
Notifications
You must be signed in to change notification settings - Fork 2
[WIP] Assign Deposit Handling #48
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
Conversation
|
Latest commit changes: Extract intention-specific logic from proposer into handlers
|
scripts/shared/db-setup.js
Outdated
| depositor TEXT NOT NULL, | ||
| token TEXT NOT NULL, | ||
| amount NUMERIC(78, 0) NOT NULL, | ||
| credited_vault TEXT, |
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.
Should remove credited_vault from the deposits table or make it into an array of credited_vaults, since there may be more than one. Probably just remove it since it's redundant and we can build the array from deposit_assignment_events.
scripts/shared/db-setup.js
Outdated
| -- Create indexes for deposits table | ||
| CREATE INDEX IF NOT EXISTS idx_deposits_tx_hash ON deposits(tx_hash); | ||
| CREATE INDEX IF NOT EXISTS idx_deposits_depositor ON deposits(depositor, chain_id); | ||
| CREATE INDEX IF NOT EXISTS idx_deposits_token ON deposits(token); |
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.
Might want to include chain_id in this index.
scripts/shared/db-setup.js
Outdated
| if (!forceDropConfirm) { | ||
| console.log(chalk.red('\n⚠️ WARNING: This will DELETE ALL DATA in the following tables:')) | ||
| console.log(chalk.red(' - bundles, cids, balances, nonces, proposers, vaults')) | ||
| console.log(chalk.red(' - bundles, cids, balances, nonces, proposers')) |
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 line should keep vaults, but also add deposit_assignment_events and deposits.
src/proposer.ts
Outdated
| */ | ||
| export async function validateVaultIdOnChain(vaultId: number): Promise<void> { | ||
| // Basic sanity | ||
| if (!Number.isInteger(vaultId) || vaultId < 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.
We should allow 0 as the vaultId since that's the protocol's vault but disallow any < 0.
| } | ||
|
|
||
| /** | ||
| * Computes block range hex strings for Alchemy getAssetTransfers requests, |
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.
Are we getting locked into an Alchemy dependency here or is this something available in generalized libraries for interacting with nodes? Okay if we accept an Alchemy dependency for the moment, just asking.
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 look into it further but all of these methods should be available and pretty similarly structured on any blockchain node/data provider !
src/proposer.ts
Outdated
| * and ingests them into the local `deposits` table via idempotent inserts. | ||
| */ | ||
| async function discoverAndIngestEthDeposits(params: { | ||
| controller: string |
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.
See earlier note about leaving controller unspecified and getting all ETH deposits from any address.
src/proposer.ts
Outdated
|
|
||
| // Note: Vault-controller mapping is created from on-chain VaultCreated events | ||
| // Seed the proposer's own vault-to-controller mapping | ||
| await seedProposerVaultMapping() |
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 did this get added back in? We should still handle this through creating a vault onchain, and then finding what vault is associated with the proposer's address as a controller.
src/proposer.ts
Outdated
| * This is crucial for allowing the proposer to sign and submit seeding intentions. | ||
| */ | ||
| // Removed seedProposerVaultMapping(); creation is handled via on-chain events | ||
| async function seedProposerVaultMapping() { |
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.
See note above.
| const input: IntentionInput = intention.inputs[i] | ||
| const output: IntentionOutput = intention.outputs[i] | ||
|
|
||
| // Optional discovery hints in input.data |
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, interesting!
| execution: [ | ||
| { | ||
| intention, | ||
| from: 0, |
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 from: 0? Should this be from the proposer's address? May be fine as-is since we're working on the proof and execution payloads as a separate task.
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.
Wait, nevermind. from is the vault number, so it should be 0, you're right.
…le log message with all table names
…from alchemy getTransfers call, add lastCheckedBlock variable
Changes since feedback
|
| } | ||
| // Ensure contracts are initialized | ||
| // Wrapper to use validator's on-chain vault ID validation with contract dependency | ||
| const validateVaultIdOnChain = async (vaultId: number): Promise<void> => { |
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 think we could fully extract these validator functions to utils/validator.js, instead of using base validator functions, but I won't be too fussy!
KagemniKarimu
left a comment
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 is some serious work! Thank you @dkuthoore :)
| const TEST_UID = (n: number) => `${TEST_TX}:${n}` | ||
| const CTRL = '0xCcCcCcCcCcCcCcCcCcCcCcCcCcCcCcCcCcCcCcCc' | ||
| const TOKEN = '0x1111111111111111111111111111111111111111' | ||
| const ZERO = '0x0000000000000000000000000000000000000000' |
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.
sidenote to maybe move these to the new test fixtures from #51 in a future PR !
Summary
This PR adds support for AssignDeposit intentions, working towards the end-to-end vault seeding flow. The node can now discover on-chain deposits (ERC-20 and ETH, both internal (from contracts) and external(from EOAs), ingest them into a new
depositstable, validate and match deposits to AssignDeposit intentions, and credit vault balances at publish time while marking deposits as assigned. The flow is fully auditable and prevents double assignment.Changes
Database
depositstable and indexes:id,tx_hash,transfer_uid(UNIQUE),chain_id,depositor(lowercased),token(ERC-20 address or 0x0 for ETH),amount(wei),credited_vault,assigned_at.deposits.Utilities (DB)
src/utils/deposits.ts:insertDepositIfMissing: Idempotent insert bytransfer_uid.findExactUnassignedDeposit: Select an exact, unassigned deposit by depositor, token, amount, chain_id.markDepositAssigned: Atomically flipassigned_atand persistcredited_vault.Proposer: Chain introspection and vault validation
validateVaultIdOnChain(vaultId): Ensures1 <= vaultId < nextVaultId()usingVaultTracker.nextVaultId().Deposit discovery and ingestion
discoverAndIngestErc20Deposits({ controller, token, chainId, fromBlockHex?, toBlockHex? })via AlchemygetAssetTransferswithcategory: ['erc20']andcontractAddresses: [token].discoverAndIngestEthDeposits({ controller, chainId, fromBlockHex?, toBlockHex? })via AlchemygetAssetTransferswithcategory: ['internal', 'external'](supports deposits from contracts and EOAs).transfer_uid(AlchemyuniqueIdif present; otherwise a deterministic fallback) to ensure idempotency.AssignDeposit validation and handler
validateAssignDepositStructure(intention):inputs[i]↔outputs[i]mapping.asset,amount,chain_id.outputs[i].to(noto_external) and validates the vault on-chain.totalFeeamounts are "0";proposerTipandprotocolFeeempty;agentTipabsent or empty.handleIntention(AssignDeposit branch):fromBlock/toBlockhints frominputs[i].data(hex).{ token, to, amount, deposit_id, depositor }.from: 0sentinel (no source vault balance is decremented for assignment).Publish-time assignment and crediting
publishBundle, for AssignDeposit executions:markDepositAssigned(deposit_id, to)to atomically assign the deposit row (guards against double assignment).amountfortoken.updateBalances(from, to, token, amount).Tests
test/integration/deposits.db.test.ts:transfer_uid.credited_vaultand setsassigned_at.Notes
getAssetTransfersfor reliability, pagination (pageKey), and structured data across ERC-20 and ETH.Next Steps
proposer.tsfile is getting really large, especially with the specific functions for handlingcreateVaultandAssignDepositintentions. My plan is to extract the logic for handling specific intentions into a dedicated directory, so the proposer.ts file is cleaner and more easily readable. This aligns with our task of defining our preliminary Intention types, we will have dedicated files for handling each of them.