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

feat: support Base resolution #262

Merged
merged 8 commits into from
Oct 22, 2024
Merged

feat: support Base resolution #262

merged 8 commits into from
Oct 22, 2024

Conversation

sammyluo
Copy link
Contributor

@sammyluo sammyluo commented Sep 24, 2024

Background

Support domain resolution for UNS domains on Base blockchain

Changes

  • Add Base configuration
  • Connect Base RPC calls
  • Update UNS and ENS config
  • Update ETH and POL testnet config

To Do

Additional Deployment and/or Rollback Steps

Code Review

This Pull Request was thoroughly reviewed and meets all Code Review Standards pertaining to:

  • Implementation - satisfied acceptance criteria
  • Communication - notified engineers/stakeholders about impactful changes
  • Error handling - handled, logged & alerted on errors
  • Documentation - wrote readable code & commits, documented unintuitive code
  • Testing - unit & E2E test coverage, tested in staging, DB migrations backwards compatible
  • Security - sanitized inputs, vetted dependencies, validated/secured API requests, no committed secrets
  • Performance - optimized expensive algorithms & database queries

Please select all applied standards relevant to this PR.

Confirmation

  • Assignee Confirmation - I (the author) have filled out the template above
  • Reviewer Confirmation - I (a/the reviewer) confirm 1) the author has filled out the template and checked the box that says Assignee Confirmation 2) I reviewed the code and selected all applicable items in the code review section.

@sammyluo sammyluo self-assigned this Sep 24, 2024
@sammyluo sammyluo marked this pull request as ready for review October 16, 2024 06:07
Copy link
Contributor

@DancingAxolotl DancingAxolotl left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for updating it!
Left a few comments to consider, but nothing that's blocking.

Comment on lines 1220 to 1234
return new Uns({
locations: {
Layer1: {
url: `${DEFAULT_UNS_PROXY_SERVICE_URL}/chains/eth/rpc`,
network: 'mainnet',
proxyServiceApiKey: config.apiKey,
},
Layer2: {
url: `${DEFAULT_UNS_PROXY_SERVICE_URL}/chains/base/rpc`,
network: 'base-mainnet',
proxyServiceApiKey: config.apiKey,
},
},
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can consider making one of the "layers" optional for cases similar to Base where we know that domains can exist only on one chain.

This will save us some RPC calls on querying L1 when we don't need to.

src/Uns.ts Outdated
Comment on lines 99 to 103
this.unsl2 = new UnsInternal(
UnsLocation.Layer2,
source.locations.Layer2,
BlockchainType.MATIC,
BlockchainType.POL,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is going to be POL for both Polygon and Base configs. It's probably fine as is, but might be worth passing the blockchain type into constructor args.

@sammyluo sammyluo force-pushed the sam/api-1177-base-support branch from 05a9313 to d495e81 Compare October 22, 2024 19:44
@sammyluo sammyluo merged commit db48ae3 into master Oct 22, 2024
5 checks passed
@sammyluo sammyluo deleted the sam/api-1177-base-support branch October 22, 2024 19:46
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