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(oracle): add oracle to get portal contract address #1474

Merged
merged 10 commits into from
Aug 9, 2023
Merged

Conversation

Maddiaa0
Copy link
Member

@Maddiaa0 Maddiaa0 commented Aug 9, 2023

Overview
closes: #1464

Adds an oracle that returns the portal address for a given contract address.
A unit test is added, and the uniswap portal test is altered to use this oracle rather than injecting the portals as function inputs

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

  • If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
  • I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
  • Every change is related to the PR description.
  • I have linked this pull request to relevant issues (if any exist).

@Maddiaa0 Maddiaa0 marked this pull request as ready for review August 9, 2023 11:50
@iAmMichaelConnor
Copy link
Contributor

I'd maybe consider doing this for all of the non-argument inputs to a circuit, or not at all. Injecting some of the call_context as an argument and some via an oracle feels inconsistent to me. #1464 (comment)

@Maddiaa0
Copy link
Member Author

Maddiaa0 commented Aug 9, 2023

I'd maybe consider doing this for all of the non-argument inputs to a circuit, or not at all. Injecting some of the call_context as an argument and some via an oracle feels inconsistent to me. #1464 (comment)

I made the pr misunderstanding the task at hand, rather than injecting the call context, the goal was to have a lookup from contract address to portal address. Ive updated the code and description to reflect this

getPortalContractAddress: async ([aztecAddress]) => {
const contractAddress = AztecAddress.fromString(aztecAddress);
let portalContactAddress = await this.contractsDb.getPortalContractAddress(contractAddress);
if (!portalContactAddress) portalContactAddress = EthAddress.ZERO;
Copy link
Member Author

Choose a reason for hiding this comment

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

Thoughts on returning zero here, or throwing. I feel like it should throw

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree -> although can you remind me how the error would bubble up to the user? (what would be their experience)

Copy link
Contributor

Choose a reason for hiding this comment

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

What practically happens if the return value from this.contractsDb.getPortalContractAddress is zero now? Because I would assume that it is already zero for contracts that don't have portals.

The main thing keeping me from saying that we should throw, would be that you would have trouble if you want to check if something have a portal and it don't have that 🤔

Copy link
Member Author

@Maddiaa0 Maddiaa0 Aug 9, 2023

Choose a reason for hiding this comment

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

good point, the user should probably have an explicit zero check,

Copy link
Member Author

@Maddiaa0 Maddiaa0 Aug 9, 2023

Choose a reason for hiding this comment

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

(foot)

image

@iAmMichaelConnor
Copy link
Contributor

I made the pr misunderstanding the task at hand, rather than injecting the call context, the goal was to have a lookup from contract address to portal address. Ive updated the code and description to reflect this

So if I'm writing code in a function of contract A, I have access to the contract address and the portal contract address of contract A, via the context already. But I can't access the portal contract address of some other contract B.
Is the purpose of this Issue/PR to enable contract A to lookup the portal contract address of contract B?

Copy link
Contributor

@LHerskind LHerskind left a comment

Choose a reason for hiding this comment

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

I like it 👀.

getPortalContractAddress: async ([aztecAddress]) => {
const contractAddress = AztecAddress.fromString(aztecAddress);
let portalContactAddress = await this.contractsDb.getPortalContractAddress(contractAddress);
if (!portalContactAddress) portalContactAddress = EthAddress.ZERO;
Copy link
Contributor

Choose a reason for hiding this comment

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

What practically happens if the return value from this.contractsDb.getPortalContractAddress is zero now? Because I would assume that it is already zero for contracts that don't have portals.

The main thing keeping me from saying that we should throw, would be that you would have trouble if you want to check if something have a portal and it don't have that 🤔

@Maddiaa0 Maddiaa0 enabled auto-merge (squash) August 9, 2023 15:37
@Maddiaa0 Maddiaa0 merged commit 5cce848 into master Aug 9, 2023
@Maddiaa0 Maddiaa0 deleted the md/get-portal branch August 9, 2023 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

feat: oracle to fetch L1 portals address from Aztec address
4 participants