Fix Filecoin FEVM token sync status handling for enabled tokens #959
Conversation
Jon-edge
changed the title
Jon-edge
force-pushed
the
jon/fix/filecoin-sync
branch
from
122bcca to
5cd305e
Compare
There was a problem hiding this comment.
It looks like token transactions were never added to this plugin. Let's add that instead
https://filfox.info/api/v1/docs/static/index.html#/default/AddressController_tokenTransferList
Jon-edge
force-pushed
the
jon/fix/filecoin-sync
branch
from
5cd305e to
0632b00
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Jon-edge
force-pushed
the
jon/fix/filecoin-sync
branch
from
0632b00 to
9cb10a6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| const ourReceiveAddresses = [] | ||
|
|
||
| // Token transfers don't have network fees in the same way as regular messages | ||
| const networkFee = '0' |
There was a problem hiding this comment.
Transfers have fees but the fees aren't visible in the transfers endpoint. https://filfox.info/api/v1/docs/static/index.html#/default/MessageController_info can get fees from a single transaction or https://filfox.info/api/v1/docs/static/index.html#/default/AddressController_messageTransferList produces a list of messages and the fee can be calculated (that endpoint shows total moved so you would have to subtract the amount to the recipient)
| @@ -51,11 +57,25 @@ export class FilfoxAdapter extends NetworkAdapter<FilfoxAdapterConfig> { | |||
| const { publicAddress: addressString } = | |||
| await this.ethEngine.getFreshAddress() | |||
|
|
|||
| const handleScanProgress = (progress: number): void => { | |||
| this.logError( | |||
There was a problem hiding this comment.
Seems like leftover debug logging, remove or comment out
| @@ -139,6 +177,197 @@ export class FilfoxAdapter extends NetworkAdapter<FilfoxAdapterConfig> { | |||
| this.ethEngine.sendTransactionEvents() | |||
| } | |||
|
|
|||
| // Initialize sync status for all enabled tokens to 0 | |||
| private initializeSyncStatusForAllTokens(): void { | |||
| this.logError( | |||
There was a problem hiding this comment.
Seems like leftover debug logging, remove or comment out
| let highestProcessedBlockHeight = startBlock | ||
|
|
||
| // Initialize sync status for all enabled tokens | ||
| this.initializeSyncStatusForAllTokens() |
There was a problem hiding this comment.
I don't understand this. Generally, sync status is used to describe the progress of initial sync and doesn't need to be reported once an asset reaches one. This change seems to make this.ethEngine.updateOnAddressesChecked() on line 89 run more frequently and for no benefit because once this.addressesChecked === true that line of code doesn't even do anything.
What's the point of doing this every time checkTransactions is called? It seems like
| } | ||
|
|
||
| // Skip transfers prior to the startBlock (avoids reprocessing during resync) | ||
| if (tokenTransfer.height <= startBlock) { |
There was a problem hiding this comment.
this check is duplicated and can be moved to where tokenTransfer is first defined
| continue | ||
| } | ||
|
|
||
| // Skip transfers prior to the last sync height |
There was a problem hiding this comment.
this check is duplicated and can be moved to where tokenTransfer is first defined
| 'FilfoxAdapter token transfers scanning failed', | ||
| error instanceof Error ? error : new Error(String(error)) | ||
| ) | ||
| // If token transfer scanning fails, mark all tokens as complete to avoid blocking sync |
There was a problem hiding this comment.
Should we try again if the fetch fails instead of faking the sync ratio?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Jon-edge
force-pushed
the
jon/fix/filecoin-sync
branch
from
5fc8fa7 to
9c45b00
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
none