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

Remove v1 deployments #308

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Remove v1 deployments #308

merged 1 commit into from
Oct 16, 2024

Conversation

guidanoli
Copy link
Collaborator

No description provided.

@guidanoli guidanoli requested a review from tuler October 2, 2024 17:22
@guidanoli guidanoli self-assigned this Oct 2, 2024
@tuler
Copy link
Member

tuler commented Oct 2, 2024

Is there any deployment left? Sepolia?

@guidanoli
Copy link
Collaborator Author

From what I see, there is no deployment left.
The export/abi and deployments directories are gone.

@tuler
Copy link
Member

tuler commented Oct 2, 2024

Oh, ok. Deroll's code needs at least 1 deployment to exist.

@guidanoli
Copy link
Collaborator Author

Why does DeRoll need at least one deployment?

@tuler
Copy link
Member

tuler commented Oct 2, 2024

Why does DeRoll need at least one deployment?

Because it uses the contracts addresses, like as an example in the utility method isERC20Deposit
https://deroll.dev/wallet/is-erc20-deposit

@guidanoli
Copy link
Collaborator Author

Maybe you don't need at least one deployment.
You could have a mapping of trusted contracts indexed by chain ID.
Remember that the chain ID is now part of the input metadata schema.

enum TrustedContractTypeID {
  Untrusted = 0,
  EtherPortal,
  ERC20Portal,
  ERC721Portal,
  ERC1155Portal,
}

// Could be implemented as a partial mapping instead of using
// 0 as a sentinel value for "untrusted"
mapping (uint256 chainId => mapping (address sender => TrustedContractTypeID)) trustedContractTypeID;

mapping (TrustedContractTypeID => AdvanceRequestHandler) trustedContractHandler;

Then, whenever it receives an advance request, it indexes this mapping with the chain ID and message sender from the metadata, and checks if it is trusted, and, if so, its type. From its type, it can forward it to a handler which can then decode it, etc.

So, without deployments info, every sender would be untrusted.

@tuler
Copy link
Member

tuler commented Oct 2, 2024

You could have a mapping of trusted contracts indexed by chain ID. Remember that the chain ID is now part of the input metadata schema.

Such mapping is being auto-generated by wagmi-cli, from deployment information in the npm package.
https://github.com/tuler/deroll/blob/main/packages/wallet/wagmi.config.ts

@guidanoli
Copy link
Collaborator Author

So it could be an issue with the wagmi CLI or with the plugin.

@tuler
Copy link
Member

tuler commented Oct 2, 2024

So it could be an issue with the wagmi CLI or with the plugin.

There is no issue with the CLI or the plugin.
If there is no deployment, then there is no address to check on. And methods like isERC20Deposit would have to be removed.

@guidanoli
Copy link
Collaborator Author

I reckon we're only removing v1 deployments to later add v2 deployments, right?
So, when we have v2 deployments, would Deroll work?

@guidanoli
Copy link
Collaborator Author

I didn't have enough funds to deploy the contracts to Ethereum Sepolia, so I deployed them to OP Sepolia instead. Beware that this is just to have Deroll not break, and that contracts might still change before the definitive v2 release.

@guidanoli guidanoli force-pushed the feature/rm-old-deployments branch from b4c3616 to c94492e Compare October 10, 2024 17:08
@guidanoli
Copy link
Collaborator Author

Removed OP Sepolia deployment, because contracts changed in the meantime.

@guidanoli
Copy link
Collaborator Author

Deroll will work because we have deployed the latest version of the contracts to Ethereum Sepolia.

@guidanoli guidanoli merged commit 31ba777 into main Oct 16, 2024
3 checks passed
@guidanoli guidanoli deleted the feature/rm-old-deployments branch October 16, 2024 16:36
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