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

Add address lookup table to find contract task/name #2031

Merged
merged 9 commits into from
Nov 28, 2022
Merged

Conversation

TomAFrench
Copy link
Contributor

@TomAFrench TomAFrench commented Nov 21, 2022

Description

builds on #2030

Given a deployment task and contract name, it's currently easy to lookup the onchain address for that contract. What's more difficult however is given an onchain contract address, check to see if it's a Balancer-deployed contract and if so return the task id and contract name.

This PR generates a lookup table for this operation which makes it easier to generate human-readable identifiers for contracts when starting from onchain data. It currently only handles contracts which are directly deployed (i.e. Pool factories but not Pools themselves) as the initial usecase for this map (tracking addresses which hold permissions on the authorizer) doesn't require this.

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Dependency changes
  • Code refactor / cleanup
  • Documentation or wording changes
  • Other

Checklist:

  • The diff is legible and has no extraneous changes
  • Complex code has been commented, including external interfaces
  • Tests are included for all code paths
  • The base branch is either master, or there's a description of how to merge

Issue Resolution

Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

I understand wanting to have this, but wouldn't a better way be to load all output files for all tasks in said network, and search for the address there? It'd mean we don't need to maintain these json files (which we'd need to check for correctness if we kept them).

@nventuro
Copy link
Contributor

We already have this task-output looping behavior when checking deployments for a network.

@TomAFrench
Copy link
Contributor Author

I understand wanting to have this, but wouldn't a better way be to load all output files for all tasks in said network, and search for the address there? It'd mean we don't need to maintain these json files (which we'd need to check for correctness if we kept them).

This is a bit nasty when you're dealing with many addresses. For every address you want to check you need to find all the deployment tasks, iterate through them loading the output json files and then iterate through every key-value pair to find one with the correct value. We need to do a lot more work than if we just precompute the result of that and save it here so we're just looking up a key in a mapping.

That said, we could use that method to compute this mapping at build-time rather than at deployment like this PR currently does. I'd still want a CI action to ensure this is up to date though.

Base automatically changed from sync-deployment-helpers to master November 22, 2022 23:10
@nventuro
Copy link
Contributor

What is the use case for this? If we don't care that much about performance, then having way simpler code seems like a good idea, instead of having to maintain the build list code, check list code, etc.

* master: (26 commits)
  Deprecate managed pool deployment (#2044)
  Add support for cash and managed to MockVault (#2064)
  chore: add .nvmrc file (#2066)
  Make `approveVault` payable (`BaseRelayerLibrary`). (#2067)
  Do new aave linear deploy (#2055)
  Deploy entrypoint (#2062)
  (Re) deployment preparation: `AuthorizerAdaptorEntrypoint` (#2060)
  chore: update deployment readmes to point to artifact files rather than abis (#2061)
  Expose deployment artifacts from deployments package rather than ABI + bytecode (#2058)
  chore: add missing action-ids (#2025)
  Remove Protocol Fee Splitter task (#2054)
  Adaptor entrypoint events (#2051)
  Verify AaveLinearPool on deployment (#2040)
  Rename IPoolVersion --> IFactoryCreatedPoolVersion. (#2045)
  docs: remove deprecated networks from jsdoc (#2029)
  Add Version unit test. (#2038)
  Update linear build info (#2041)
  Make unnecessarily async functions sync (#2030)
  Do deployment (#2035)
  Aave linear pool versioning (#2032)
  ...
@TomAFrench
Copy link
Contributor Author

I think shifting this code to index.ts would increase code complexity. We have a lot of helpers for manipulating the task data which we can use internally but whereas we'd get more brittle and harder to maintain code by redoing this in index.ts

@TomAFrench
Copy link
Contributor Author

What is the use case for this?

The initial usecase is for it to be used in the permissions package in so we can label addresses with permissions more easily.

pkg/deployments/index.ts Show resolved Hide resolved
pkg/deployments/index.ts Show resolved Hide resolved
pkg/deployments/src/network.ts Show resolved Hide resolved
pkg/deployments/src/network.ts Show resolved Hide resolved
@TomAFrench TomAFrench merged commit 59ac549 into master Nov 28, 2022
@TomAFrench TomAFrench deleted the address-lookup branch November 28, 2022 21:22
@TomAFrench TomAFrench mentioned this pull request Nov 28, 2022
11 tasks
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.

2 participants