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

Add Filecoin FEVM #662

Merged
merged 22 commits into from
Nov 14, 2023
Merged

Add Filecoin FEVM #662

merged 22 commits into from
Nov 14, 2023

Conversation

samholmes
Copy link
Collaborator

@samholmes samholmes commented Nov 8, 2023

Now that the call signature for EthereumNetwork.multicastRpc matches
NetworkAdaptor.multicastRpc, there is no need for the case handling
for each method.

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Description

none

Copy link
Contributor

@swansontec swansontec left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up. Can you move the Filecoin commits to the end of the pull request? That way we just have some clean refactoring, followed by a new currency that uses the fresh features.

// {
// filfoxUrl: 'https://filfox.info/api/v1',
// filscanUrl: 'https://api-v2.filscan.io/api/v1',
// hdPathCoinType: 461,
Copy link
Contributor

Choose a reason for hiding this comment

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

hdPathCoinType already exists below.

src/ethereum/info/filecoinFevmCalibrationInfo.ts Outdated Show resolved Hide resolved
},

async getInnerPlugin() {
/* webpackChunkName: "filecoinfevmcalibration" */
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this webpackChunkName. The ethereumInfo file defines the chunk name for the Ethereum sources, so this line will conflict with that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought ethereumInfo was a peer of all the other EVM info files. Looks like you're right that those other info files don't have the webpackChunkName set for them, but I'm curious as to why? Wouldn't it make sense to chunk each info file as they're slurped up by ethereumInfos.ts (the index file of all the other EVM infos, not the info file for Ethereum)?

Copy link
Contributor

@swansontec swansontec Nov 10, 2023

Choose a reason for hiding this comment

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

The webpackChunkName comment gives the final name of the file being imported in this import() expression. So we are saying that, once bundled on disk, the "EthereumTools.ts" file should be called "ethereum-[hash].js". This bundled file will include "EthereumTools.ts" (of course), but also any unique transitive dependencies not shared with other chunks. So we would expect files like "EthereumEngine.ts" to also get bundled into "ethereum-[hash].js", along with some SDK bits from node_modules.

This is completely unnecessary on a technical level. Nobody is going to see the file names that get embedded into the app, and filenames don't matter to the WebView either. However, it does sometimes help with debugging packaging problems and with better stack traces, so it's worth doing.

src/ethereum/info/filecoinFevmInfo.ts Outdated Show resolved Hide resolved
src/ethereum/info/filecoinFevmInfo.ts Outdated Show resolved Hide resolved
[method in UpdateMethods]: Array<
(...args: any[]) => Promise<EthereumNetworkUpdate>
>
type NetworkAdaptorUpdateMethod = 'blockheight' | 'nonce' | 'tokenBal' | 'txs'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be keyof NetworkAdaptor, instead of hard-coding it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Later in the PR, not all keys are update methods. Perhaps I can couple the types though using TS utility types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that makes sense.

src/ethereum/EthereumNetwork.ts Outdated Show resolved Hide resolved
src/ethereum/EthereumNetwork.ts Outdated Show resolved Hide resolved
src/ethereum/networkAdaptors/types.ts Outdated Show resolved Hide resolved
src/ethereum/networkAdaptors/FilfoxAdaptor.ts Outdated Show resolved Hide resolved
@swansontec swansontec changed the title Add Filecoin EVEM Add Filecoin FEVM Nov 10, 2023
Copy link
Contributor

@swansontec swansontec left a comment

Choose a reason for hiding this comment

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

Excellent work!

@samholmes samholmes force-pushed the sam/filecoin-fevm branch 2 times, most recently from bcb0a9a to 88fd389 Compare November 13, 2023 23:12
@samholmes samholmes enabled auto-merge November 13, 2023 23:12
@samholmes samholmes disabled auto-merge November 13, 2023 23:24
This makes the method names clearer that they're are retrieval methods.
Now that the call signature for `EthereumNetwork.multicastRpc` matches
`NetworkAdaptor.multicastRpc`, there is no need for the case handling
for each method.
@samholmes samholmes enabled auto-merge November 13, 2023 23:38
@samholmes samholmes merged commit 8ca9e9a into master Nov 14, 2023
3 checks passed
@samholmes samholmes deleted the sam/filecoin-fevm branch November 14, 2023 19:05
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