palomachain / paloma

The fast blockchain messenger protocol

Apache License 2.0

290 stars 135 forks source link

chore: disable token relink before compass 2.0 #1246

Closed maharifu closed 3 months ago

maharifu commented 3 months ago

Related Github tickets

https://github.com/VolumeFi/paloma/issues/1891

Background

Even if we don't want to move test tokens, since we have them set, paloma will still try to move them. This will fail since (new) pigeon won't be compatible with (old) compass, so we need to disable this functionality before compass 2.0 is deployed and re-enable it again afterwards.

Testing completed

[ ] test coverage exists or has been added/updated
[ ] tested in a private testnet

Breaking changes

[ ] I have checked my code for breaking changes
[ ] If there are breaking changes, there is a supporting migration.

byte-bandit commented 3 months ago

@maharifu Instead of de- and re-activating this during the upgrade path, what about if we removed support for the chains and re-added them instead?

maharifu commented 3 months ago

@maharifu Instead of de- and re-activating this during the upgrade path, what about if we removed support for the chains and re-added them instead?

That would be cleaner, but it would also take longer, since we have to do by governance vote, right? Also, are there any other side effects from removing support for a chain?

byte-bandit commented 3 months ago

@maharifu Instead of de- and re-activating this during the upgrade path, what about if we removed support for the chains and re-added them instead?

That would be cleaner, but it would also take longer, since we have to do by governance vote, right? Also, are there any other side effects from removing support for a chain?

I'm not 100% sure tbh. Messages should be drained automatically, but I'm unclear about deployed tokens and whether they're cleaned up or not. Would need some investigation.

You're right about the additional downtime though :/

maharifu commented 3 months ago

@maharifu Instead of de- and re-activating this during the upgrade path, what about if we removed support for the chains and re-added them instead?

That would be cleaner, but it would also take longer, since we have to do by governance vote, right? Also, are there any other side effects from removing support for a chain?

I'm not 100% sure tbh. Messages should be drained automatically, but I'm unclear about deployed tokens and whether they're cleaned up or not. Would need some investigation.

You're right about the additional downtime though :/

I'll try to test this in the private testnet and see what happens. It's not a guarantee, but should highlight obvious issues, if any. We can leave the PR open in the meanwhile.

maharifu commented 3 months ago

@byte-bandit The ERC20 tokens are independent of the supported chains, so removing support for a chain does not remove the ERC20 tokens.

Worse still, there's currently a logic bug in the token relink function, such that if we only have ERC20 tokens for unsupported chains when trying to attest a new compass upload, we will start to relink, but not do anything. The deployment cache won't have the msgID and the contract won't become active.

We can create a new vote to remove ERC20 tokens, which we will probably need anyway in the future, but it would need a new release and a new vote.

maharifu commented 3 months ago

Let's take the pain then. Can you setup a reminder ticket so we don't forget to re-enable this later on please? Also worth taking a note of the bug you found.

Sure. I already have a ticket for the bug - https://github.com/VolumeFi/paloma/issues/1953 - and created a new one to revert this PR - https://github.com/VolumeFi/paloma/issues/1956