Skip to content

Conversation

@mkflow27
Copy link
Collaborator

@mkflow27 mkflow27 commented May 2, 2025

Working on automating the error code updates.

@vercel
Copy link

vercel bot commented May 2, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs-v3 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 12, 2025 9:08am

| `MulOverflow()` | `0x0cde6c26` | | MockGyroECLPPool |
| `DivInterval()` | `0xe03f5d57` | | MockGyroECLPPool |
| `OwnableUnauthorizedAccount(address)` | `0x118cdaa7` | address account | MockLBPool |
| `OwnableInvalidOwner(address)` | `0x1e4fbdf7` | address owner | MockLBPool |
Copy link
Contributor

@EndymionJkb EndymionJkb May 2, 2025

Choose a reason for hiding this comment

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

InvalidOwner is in the LBPPoolFactory; not sure where it's getting the "Ownable" from. We don't use that OZ contract directly (with LBPs, anyway; some of the hooks do).

| `ContractNameAlreadyRegistered(ContractType,)` | `0xdecd1563` | ContractType contractType, string contractName | MevCaptureHook |
| `ContractNameInUseAsAlias(string,)` | `0xedcd5939` | string contractName, address contractAddress | MevCaptureHook |
| `ContractAliasInUseAsName(ContractType,)` | `0xc5949bff` | ContractType contractType, string contractName | MevCaptureHook |
| `ContractNameNotRegistered(string)` | `0xcd3599f9` | string contractName | MevCaptureHook |
Copy link
Contributor

@EndymionJkb EndymionJkb May 2, 2025

Choose a reason for hiding this comment

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

ContractNameNotRegistered is in the BalancerContractRegistry

| `AmplificationFactorTooLow()` | `0xab923323` | | MockStablePool |
| `AmplificationFactorTooHigh()` | `0x9b80d390` | | MockStablePool |
| `AmpUpdateDurationTooShort()` | `0xcd6b022a` | | MockStablePool |
| `AmpUpdateRateTooFast()` | `0x1c708b92` | | MockStablePool |
Copy link
Contributor

Choose a reason for hiding this comment

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

The AmpUpdate errors are in regular StablePool - are you doing this by reading the ABI, vs. reading the contracts? There really shouldn't be any "Mock" contracts. (They sometimes do declare their own errors, but just for tests, so I don't think they need to be documented.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have moved to taking the abi from the deployments repo now. The errors should now match what is actually deployed onchain

@mkflow27
Copy link
Collaborator Author

@EndymionJkb I have updated the way how I fetch & generate the error codes. I now use some (not all) of the contracts in the deployment repo. I could add all the remaining ones and update the pr, but figured for now the relevant ones are included.

Let me know what you think about it.

@EndymionJkb
Copy link
Contributor

EndymionJkb commented Jun 14, 2025

@EndymionJkb I have updated the way how I fetch & generate the error codes. I now use some (not all) of the contracts in the deployment repo. I could add all the remaining ones and update the pr, but figured for now the relevant ones are included.

Let me know what you think about it.

Does this really need to be automated? Most of it is static - once you have the full set (which we already have in the existing document), there should only be additions.

Maybe whenever there's a production deployment, one of the checklist items will be "add any new errors to the docs" (or at least "inform <whoever's in charge of docs> that there's been a deployment that adds new errors".

They're custom errors that are generally displayed in the console/Tenderly UI; when do we need to decode them from signatures?

Also, it's picking up things that don't exist, like the very first two: AddressEmptyCode, AddressInsufficientBalance, and we lose information about where the error is defined (e.g. which interface or contract).

Are you proposing replacing the existing error page with this? Or just adding this? I guess it's fine if you just add it, but I'm not sure it's useful. Maybe put the motivation / use case in the description.

@mkflow27
Copy link
Collaborator Author

mkflow27 commented Jun 16, 2025

@EndymionJkb I have updated the way how I fetch & generate the error codes. I now use some (not all) of the contracts in the deployment repo. I could add all the remaining ones and update the pr, but figured for now the relevant ones are included.

Let me know what you think about it.

Does this really need to be automated? Most of it is static - once you have the full set (which we already have in the existing document), there should only be additions.

Maybe whenever there's a production deployment, one of the checklist items will be "add any new errors to the docs" (or at least "inform <whoever's in charge of docs> that there's been a deployment that adds new errors".

They're custom errors that are generally displayed in the console/Tenderly UI; when do we need to decode them from signatures?

Also, it's picking up things that don't exist, like the very first two: AddressEmptyCode, AddressInsufficientBalance, and we lose information about where the error is defined (e.g. which interface or contract).

Are you proposing replacing the existing error page with this? Or just adding this? I guess it's fine if you just add it, but I'm not sure it's useful. Maybe put the motivation / use case in the description.

Does this really need to be automated? Most of it is static - once you have the full set (which we already have in the existing document), there should only be additions.

It does not need to be automated no, the main reason I need to work on this right now is that the signatures are wrong, apparently. forge inspect /pkg/vault/contracts/Vault.sol errors gives other signatures than what is currently live. This is the case for other contracts as well.

They're custom errors that are generally displayed in the console/Tenderly UI; when do we need to decode them from signatures?

This happens regularly if one does fork-tests with Balancer when not working with the monorepo, or say doing some circumstances with the sdk. Other people and our team have found it helpful to have decoded errors available.

Also, it's picking up things that don't exist, like the very first two: AddressEmptyCode, AddressInsufficientBalance, and we lose information about where the error is defined (e.g. which interface or contract).

Not sure what you mean here. in this build info the error "AddressInsufficientBalance" and "AddressEmptyCode" are defined. If you have a better way of fetching the relevant errors, let me know. I initially took them from the monorepo.

Are you proposing replacing the existing error page with this? Or just adding this? I guess it's fine if you just add it, but I'm not sure it's useful. Maybe put the motivation / use case in the description.

I am proposing replacing the current page with this. It just takes all the error codes from the deployment artifacts and creates the signatures and adds them.

@EndymionJkb
Copy link
Contributor

EndymionJkb commented Jun 16, 2025

in this build info the error "AddressInsufficientBalance" and "AddressEmptyCode" are defined
The build-info includes all the imported libraries. Those are OpenZeppelin errors.

I am proposing replacing the current page with this. It just takes all the error codes from the deployment artifacts and
creates the signatures and adds them.

My problem with that is it loses information. To me the important thing is to know which interface the error came from and the text from the notice tag so that you know what it means without having to search the code for it (in vain, if it's AddressEmptyCode and not in our codebase).

The extra thing it has is the signature, which you've explained is useful. The regular error codes doc already has the text of the signatures. You can just keccak256 those and take the first 4 bytes, without going into the build-info at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants