-
Notifications
You must be signed in to change notification settings - Fork 75
feat: use factory proxy address as AA wallet deriviation to remain unchanged during implemenation upgrades #431
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
…ry implementation address - Update Base Sepolia Wallet Implementation to 0xf5d0c65516f0724242343c4eAA5D9de3ee4291fB - Update Base Sepolia Factory Implementation to 0x50CcaD1dAAd3e2Aac91d5a9c7425f9cA3a119b13 - Update Base Sepolia Paymaster to 0xd856f532F7C032e6b30d76F19187F25A068D6d92 - Correct Sepolia Factory Implementation address to actual on-chain address 0x8d2c0e4ace9e674d6066b0681ce1af95e5e19864
- Update aa.go to read factory address from config with default fallback - Make SetFactoryAddress() functional to store factory address from config - Remove redundant SetFactoryAddress() calls from tests (engine sets it automatically) - Fix Tenderly credential loading functions to call loadTestConfigOnce()
- Modified ListWallets to store the default derived wallet (salt:0) in the database when it doesn't exist - Added comprehensive unit tests to verify wallet storage behavior: - TestListWalletsStoresDefaultWallet: Tests storage of salt:0 wallet for new owner - TestListWalletsDoesNotDuplicateExistingWallet: Tests no duplication when wallet exists - TestListWalletsWithMultipleWallets: Tests storage across multiple GetWallet and ListWallets calls - All tests verify database state directly using GetWallet and GetByPrefix - Ensures all smart wallet addresses returned to clients are tracked in the database
…AA implementation
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.
Pull Request Overview
This pull request introduces improvements to smart wallet address derivation, wallet storage management with limits enforcement, and operator tooling support. The main changes focus on making wallet address computation more deterministic, adding storage limits for wallets per owner, and expanding build system support for Base Sepolia operations.
Key Changes:
- Refactored smart wallet address computation to use factory proxy address for deterministic derivation
- Added wallet storage limits per owner with configurable maximums
- Introduced Base Sepolia operator commands in the Makefile
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/Contract.md | Updated contract addresses for Factory Implementation and Paymaster contracts across networks, clarifying which addresses are used for each network |
| core/testutil/utils.go | Added loadTestConfigOnce() calls to Tenderly-related functions for defensive initialization |
| core/taskengine/vm_runner_contract_write_simulation_test.go | Removed redundant factory address setting calls, relying on automatic initialization by engine |
| core/taskengine/list_wallets_storage_test.go | Added comprehensive test suite for wallet storage behavior in ListWallets operation |
| core/taskengine/engine.go | Implemented wallet storage logic in ListWallets with limit enforcement and default wallet persistence |
| core/chainio/aa/aa.go | Refactored address derivation with local computation and on-chain fallback, added thread-safe factory address management |
| aggregator/auth.go | Changed authentication to use smart wallet chain ID instead of global EigenLayer chain ID |
| Makefile | Added operator-base-sepolia command for Base Sepolia network operations |
Comments suppressed due to low confidence (1)
core/taskengine/engine.go:535
- When the wallet limit is reached (line 477-479), the default wallet is skipped from storage but still added to the response (lines 530-535). This creates an inconsistency where the client receives a wallet address that doesn't exist in the database.
This could cause issues if:
- The client tries to use this wallet for subsequent operations
- Other operations expect the wallet to be in the database
Consider either:
- Not returning the wallet at all if it can't be stored (remove from response when limit reached)
- OR storing it anyway but marking it with a special flag
- OR returning an error to the client indicating the limit has been reached
The current behavior of silently returning an unstored wallet may lead to confusing errors later.
if len(unique) >= maxAllowed {
// Skip adding the default wallet if limit is reached, but log a warning
n.logger.Warn("Max wallet count reached for owner in ListWallets, skipping default wallet storage", "owner", owner.Hex(), "limit", maxAllowed)
} else {
// Store the default wallet since we're returning it to the client
n.logger.Info("Default wallet not found in DB for ListWallets, storing it", "owner", owner.Hex(), "walletAddress", defaultDerivedAddress.Hex())
newModelWallet := &model.SmartWallet{
Owner: &owner,
Address: defaultDerivedAddress,
Factory: &defaultSystemFactory,
Salt: defaultSalt,
IsHidden: false,
}
if storeErr := StoreWallet(n.db, owner, newModelWallet); storeErr != nil {
n.logger.Error("Error storing default wallet to DB for ListWallets", "owner", owner.Hex(), "walletAddress", defaultDerivedAddress.Hex(), "error", storeErr)
// Continue and return the wallet anyway, but log the error
} else {
// Successfully stored, update modelWallet for response
modelWallet = newModelWallet
}
}
}
} else {
// No config but wallet not in DB - store it anyway
n.logger.Info("Default wallet not found in DB for ListWallets, storing it", "owner", owner.Hex(), "walletAddress", defaultDerivedAddress.Hex())
newModelWallet := &model.SmartWallet{
Owner: &owner,
Address: defaultDerivedAddress,
Factory: &defaultSystemFactory,
Salt: defaultSalt,
IsHidden: false,
}
if storeErr := StoreWallet(n.db, owner, newModelWallet); storeErr != nil {
n.logger.Error("Error storing default wallet to DB for ListWallets", "owner", owner.Hex(), "walletAddress", defaultDerivedAddress.Hex(), "error", storeErr)
// Continue and return the wallet anyway, but log the error
} else {
// Successfully stored, update modelWallet for response
modelWallet = newModelWallet
}
}
} else {
n.logger.Warn("DB error fetching default derived wallet for ListWallets", "address", defaultDerivedAddress.Hex(), "error", dbGetErr)
}
// Always include the default wallet in the response (even if storage failed)
if modelWallet != nil {
isHidden = modelWallet.IsHidden
actualSalt = modelWallet.Salt.String()
if modelWallet.Factory != nil {
actualFactory = modelWallet.Factory.Hex()
}
}
walletsToReturnProto = append(walletsToReturnProto, &avsproto.SmartWallet{
Address: defaultDerivedAddress.Hex(),
Salt: actualSalt,
Factory: actualFactory,
IsHidden: isHidden,
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This pull request introduces several improvements and fixes related to smart wallet management, configuration, and operator support. The main changes enhance the stability and configurability of smart wallet address derivation, improve the handling of wallet storage limits, and add support for new operator commands in the build system.
Smart wallet address derivation and configuration
core/chainio/aa/aa.goto use a deterministic approach based on the factory proxy address, owner address, and salt, ensuring stability across factory upgrades. AddedSetFactoryAddress,getFactoryAddress, and related locking for thread safety. ([[1]](https://github.com/AvaProtocol/EigenLayer-AVS/pull/431/files#diff-46ee80d910393736234820d6ef77ee18db6b60ef524efe5dce7c9645d31a2aaeR30-R57),[[2]](https://github.com/AvaProtocol/EigenLayer-AVS/pull/431/files#diff-46ee80d910393736234820d6ef77ee18db6b60ef524efe5dce7c9645d31a2aaeL65-R145))[core/chainio/aa/aa.goL65-R145](https://github.com/AvaProtocol/EigenLayer-AVS/pull/431/files#diff-46ee80d910393736234820d6ef77ee18db6b60ef524efe5dce7c9645d31a2aaeL65-R145))aggregator/auth.goto use the smart wallet chain ID (r.chainID) instead of a global chain ID, ensuring correct chain selection for authentication. ([aggregator/auth.goL316-R321](https://github.com/AvaProtocol/EigenLayer-AVS/pull/431/files#diff-cc9769ad88ccae45b8ffdb6a6fe9524ac9a7f4db2d82dadf6fbdc269f58eea58L316-R321))Wallet storage and limits
core/taskengine/engine.gofor storing smart wallets when listing wallets: enforces maximum wallet count per owner, logs warnings when limits are reached, and ensures the default wallet is always included in responses. ([core/taskengine/engine.goL456-R529](https://github.com/AvaProtocol/EigenLayer-AVS/pull/431/files#diff-77494b7116e328f80878e42128327bd124ab4ef1d7394c1d51109a5a5e93c4bcL456-R529))Build system and operator support
operator-base-sepoliacommand to theMakefile, allowing operators to start with the Base Sepolia configuration, including usage documentation and log output. ([[1]](https://github.com/AvaProtocol/EigenLayer-AVS/pull/431/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L176-R184),[[2]](https://github.com/AvaProtocol/EigenLayer-AVS/pull/431/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R202-R206))Miscellaneous
core/chainio/aa/aa.go. ([[1]](https://github.com/AvaProtocol/EigenLayer-AVS/pull/431/files#diff-46ee80d910393736234820d6ef77ee18db6b60ef524efe5dce7c9645d31a2aaeR8-R18),[[2]](https://github.com/AvaProtocol/EigenLayer-AVS/pull/431/files#diff-46ee80d910393736234820d6ef77ee18db6b60ef524efe5dce7c9645d31a2aaeL40-R77))