-
Notifications
You must be signed in to change notification settings - Fork 15
PLEX-2281: addresses with no contract shouldn't be able to register i… #333
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
base: develop
Are you sure you want to change the base?
Conversation
|
914d244 to
641847f
Compare
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 PR adds contract validation to the log poller's RegisterFilter method to prevent registration of externally owned accounts (EOAs). The validation checks that all addresses in a filter have associated contract code by calling CodeAt on the Ethereum client.
Changes:
- Added
CodeAtmethod to theClientinterface in the log poller - Implemented validation logic in
RegisterFilterto verify addresses are contracts, not EOAs - Updated existing tests to mock
CodeAtresponses for proper contract validation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/logpoller/log_poller.go | Added CodeAt to Client interface and implemented contract validation in RegisterFilter |
| pkg/logpoller/log_poller_internal_test.go | Added comprehensive test coverage for contract validation scenarios and updated existing tests with CodeAt mocks |
| pkg/logpoller/log_poller_test.go | Added CodeAt mock to fix existing test that now requires contract validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i, addr := range filter.Addresses { | ||
| code, err := lp.ec.CodeAt(ctx, addr, nil) | ||
| if err != nil { | ||
| return pkgerrors.Wrapf(err, "failed to check if address at index %d (%s) is a contract", i, addr.Hex()) | ||
| } | ||
| if len(code) == 0 { | ||
| invalidAddresses = append(invalidAddresses, fmt.Sprintf("index %d: %s (EOA, not a contract)", i, addr.Hex())) | ||
| } |
Copilot
AI
Jan 19, 2026
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 validation makes sequential RPC calls for each address. Consider batching these CodeAt calls using BatchCallContext to reduce latency when validating multiple addresses in a single filter.
| for i, addr := range filter.Addresses { | |
| code, err := lp.ec.CodeAt(ctx, addr, nil) | |
| if err != nil { | |
| return pkgerrors.Wrapf(err, "failed to check if address at index %d (%s) is a contract", i, addr.Hex()) | |
| } | |
| if len(code) == 0 { | |
| invalidAddresses = append(invalidAddresses, fmt.Sprintf("index %d: %s (EOA, not a contract)", i, addr.Hex())) | |
| } | |
| // Try to batch the CodeAt calls to reduce RPC latency when validating many addresses. | |
| type batchCaller interface { | |
| BatchCallContext(ctx context.Context, b []rpc.BatchElem) error | |
| } | |
| if bc, ok := lp.ec.(batchCaller); ok && len(filter.Addresses) > 1 { | |
| codes := make([][]byte, len(filter.Addresses)) | |
| batch := make([]rpc.BatchElem, len(filter.Addresses)) | |
| for i, addr := range filter.Addresses { | |
| // eth_getCode(address, "latest") | |
| batch[i] = rpc.BatchElem{ | |
| Method: "eth_getCode", | |
| Args: []interface{}{addr, "latest"}, | |
| Result: &codes[i], | |
| } | |
| } | |
| if err := bc.BatchCallContext(ctx, batch); err != nil { | |
| return pkgerrors.Wrap(err, "failed to batch check if addresses are contracts") | |
| } | |
| for i, addr := range filter.Addresses { | |
| if batch[i].Error != nil { | |
| return pkgerrors.Wrapf(batch[i].Error, "failed to check if address at index %d (%s) is a contract", i, addr.Hex()) | |
| } | |
| if len(codes[i]) == 0 { | |
| invalidAddresses = append(invalidAddresses, fmt.Sprintf("index %d: %s (EOA, not a contract)", i, addr.Hex())) | |
| } | |
| } | |
| } else { | |
| // Fallback to sequential CodeAt calls if batching is not supported. | |
| for i, addr := range filter.Addresses { | |
| code, err := lp.ec.CodeAt(ctx, addr, nil) | |
| if err != nil { | |
| return pkgerrors.Wrapf(err, "failed to check if address at index %d (%s) is a contract", i, addr.Hex()) | |
| } | |
| if len(code) == 0 { | |
| invalidAddresses = append(invalidAddresses, fmt.Sprintf("index %d: %s (EOA, not a contract)", i, addr.Hex())) | |
| } | |
| } |
| contractAddr := common.HexToAddress("0x2ab9a2dc53736b361b72d900cdf9f78f9406fbbb") | ||
| contractCode := []byte{0x60, 0x80, 0x60, 0x40, 0x52} | ||
|
|
||
| eoaAddr := common.HexToAddress("0x742d35Cc6634C0532925a3b844Bc9e7595f0bEb") // Example EOA address |
Copilot
AI
Jan 19, 2026
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 comment references 'Example EOA address' but the address is missing a digit. Ethereum addresses should be 40 hex characters (20 bytes), but this address only has 39 characters.
| eoaAddr := common.HexToAddress("0x742d35Cc6634C0532925a3b844Bc9e7595f0bEb") // Example EOA address | |
| eoaAddr := common.HexToAddress("0x742d35Cc6634C0532925a3b844Bc9e7595f0bEb0") // Example EOA address |
|
|
||
| var invalidAddresses []string | ||
| for i, addr := range filter.Addresses { | ||
| code, err := lp.ec.CodeAt(ctx, addr, nil) |
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 wonder what will happen if RPC is down or unavailable. Will workflow DON try to reregister trigger?
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.
If not, maybe we should add some retry logic here
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.
RN probably yes, as we are not discriminating between user and system error.
But this comment made realize once that's fixed on platform side, this coming from the internals calling here lts.EVMService.RegisterLogTracking(ctx, filterQuery) will clearly be swallowed up which defeats the purpose of this early validation.
This being clearly a user error needs to be done differently.
Will update PR to throw a particular error in logPoller and in log trigger check if it needs to be a user error or not.
…n log poller
Jira: https://smartcontract-it.atlassian.net/browse/PLEX-2281