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

refactor: Remove RpcAsset (PRO-1187) #4491

Merged
merged 9 commits into from
Feb 15, 2024
Merged

refactor: Remove RpcAsset (PRO-1187) #4491

merged 9 commits into from
Feb 15, 2024

Conversation

AlastairHolmes
Copy link
Contributor

@AlastairHolmes AlastairHolmes commented Feb 6, 2024

This changes the serde impls of any::Asset, to match those of the existing RpcAsset, and then replaces every usage of RpcAsset with Asset. Also in the places we previously used the serde impl of Asset, I've added a OldAsset type to provide matching behaviour (so we don't break the interfaces). The existing serde impl on Asset, only allowed "Eth"/"Usdc" etc, which is not what we want so we will need to replace those uses of OldAsset, with v2 rpcs etc at some point, which I will create a ticket to do.

This also introduces the AssetMap types, that provide compile time safe "asset->item" maps.

In a few of the CLI commands we allow parameters like source_asset, and source_chain. I removed this option, as I feel strongly using separate arguments to allow users to explicitly specify the chain is not the right approach. If we care about this functionality, I'd suggest we use something allow something like: "Ethereum:USDC" (Just for the CLI). (I can change this back, but I feel the current design doesn't work nicely with the direction we intend to go with always specifying the chain).

I'm going to replace Asset with ForeignChainAndAsset (of course renaming it to Asset), but will do so later. As this PR makes sense on its own (And doing so will take abit more time then I'd like to spend atm).

Note this is split into atomic commits:

  • First I change the serde impl
  • Then I remove rpc asset type, and replace it with the new Asset type
  • Then introduce the AssetMap type, replacing the existing HashMap<ForeignChain, HashMap<Asset, T>> we use.
  • Then find all the locations we were using the Old Asset's serde impl (which we probably shouldn't have been doing), and replace those cases with the OldAsset type (with has matching behaviour to the old Asset type).

Finally the only breaking change in this PR, is the change to the CLI mentioned above, all the other interfaces are unchanged, but I had to change one of the snapshots because in the unit tests previous in the HashMap<ForeignChain, HashMap<Asset, T>> we didn't include all the assets and chains, but my new AssetMaps always have all assets/chains, but in all the real interfaces this is true always anyway.

squash

Add back chain::Asset serde impls (Needed for GenesisConfig `dust_limits`)

squash

squash

squash

squash Serde

squash serde
squash

squash

squash remove rpc asset
squash asssetmap

squash
@AlastairHolmes AlastairHolmes marked this pull request as ready for review February 14, 2024 05:26
@AlastairHolmes AlastairHolmes requested a review from a team as a code owner February 14, 2024 05:26
@AlastairHolmes AlastairHolmes requested review from jerryafr and acdibble and removed request for a team February 14, 2024 05:26
@AlastairHolmes
Copy link
Contributor Author

AlastairHolmes commented Feb 14, 2024

Note there is one other breaking change, to the ingress-egress tracker, but I agreed with @acdibble that this is ok. The change is moving from the OldAsset encoding to the new RpcAsset style encoding.

@AlastairHolmes
Copy link
Contributor Author

Also I added the option to do "{"asset": "ETH"}", which I think makes sense while we allow the chain to be implicit. But happy to change back.

Copy link

codecov bot commented Feb 14, 2024

Codecov Report

Attention: 205 lines in your changes are missing coverage. Please review.

Comparison is base (9440e2a) 73% compared to head (46d305d) 73%.
Report is 3 commits behind head on main.

❗ Current head 46d305d differs from pull request most recent head 31f2039. Consider uploading reports for the commit 31f2039 to get more accurate results

Files Patch % Lines
state-chain/custom-rpc/src/lib.rs 35% 98 Missing ⚠️
state-chain/primitives/src/chains/assets.rs 63% 52 Missing and 9 partials ⚠️
state-chain/runtime/src/lib.rs 0% 0 Missing and 33 partials ⚠️
api/lib/src/queries.rs 0% 5 Missing ⚠️
api/lib/src/lp.rs 0% 4 Missing ⚠️
state-chain/runtime/src/chainflip.rs 33% 4 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #4491    +/-   ##
======================================
- Coverage     73%     73%    -0%     
======================================
  Files        401     401            
  Lines      68276   68048   -228     
  Branches   68276   68048   -228     
======================================
- Hits       49697   49445   -252     
- Misses     15922   15972    +50     
+ Partials    2657    2631    -26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

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

Very nice.

Wen ChainMap 😆 ?

Would be good to talk about how this extends to new chains (ie. arbitrum: ArbEth vs (Arb, Eth) etc.

state-chain/primitives/src/chains/assets.rs Show resolved Hide resolved
state-chain/primitives/src/chains/assets.rs Show resolved Hide resolved
@AlastairHolmes AlastairHolmes enabled auto-merge (squash) February 15, 2024 10:46
@AlastairHolmes AlastairHolmes merged commit 76ce86a into main Feb 15, 2024
42 checks passed
@AlastairHolmes AlastairHolmes deleted the refactor/PRO-1187 branch February 15, 2024 11:14
syan095 added a commit that referenced this pull request Feb 16, 2024
…igned-error

* origin/main:
  chore: fetch solana state from docker (#4538)
  feat: charge a fee for opening swap deposit addresses (#4512)
  refactor: Remove RpcAsset (PRO-1187) (#4491)
  fix: re-add version update (#4533)
  fix: pull first (#4526)
  feat: auto pick non-breaking changes (#4498)

# Conflicts:
#	state-chain/pallets/cf-ingress-egress/src/lib.rs
#	state-chain/pallets/cf-ingress-egress/src/tests.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants