-
Notifications
You must be signed in to change notification settings - Fork 421
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
feat: FuelVM support for Hyperlane #4861
base: main
Are you sure you want to change the base?
feat: FuelVM support for Hyperlane #4861
Conversation
…structure/hyperlane-monorepo into feat/fuel-integration
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4861 +/- ##
=======================================
Coverage 77.53% 77.53%
=======================================
Files 103 103
Lines 2110 2110
Branches 190 190
=======================================
Hits 1636 1636
Misses 453 453
Partials 21 21
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments; e2e as part of this PR will be necessary for approval but shouldn't be a blocker for audit.
} | ||
|
||
// Implement `EventDataTransformer` for `GasPaymentEvent` | ||
impl EventDataTransformer for GasPaymentEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These implementations all look identical, is it possible to make them more generic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// These conversions are the `From` implementations for converting the `Receipt` schema from the Fuels Rust SDK | ||
// since we cannot implement `From` for our custom Recipt schema on the `fuels::tx::Receipt` directly. | ||
|
||
pub fn generate_receipt(schema: Receipt) -> Result<FuelRecepit, ConversionError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda suprised there's no library for this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: FuelRecepit spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No library as from what I've seen, the types are ripped from the FuelSDK and redone since the types they have do not support all the GraphQL operations that are used to make the indexer optimized. The only supported ones are the ones which the SDK exposes as functions and chaining those together to make the indexer work is very inefficient.
Even though this code is quite ugly, it allows us to index Fuel using a single query, which is great for not spamming RPC requests.
.into(), | ||
), | ||
pc: schema | ||
.pc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curosity, what's pc here - program counter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a hexadecimal string representation of a 64-bit unsigned integer; value of register $pc.
} | ||
|
||
impl FuelInterchainGasPaymaster { | ||
/// Create a new fuel validator announce contract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validator announce?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[cynic::schema("fuel")] | ||
mod schema {} | ||
|
||
// The following macros and conversions are copied from the Fuels Rust SDK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we not import this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's needed for the schema builder
.with_tx_policies(tx_policies) | ||
.determine_missing_contracts(Some(3)) | ||
.determine_missing_contracts(Some(10)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make 10 a constant as max reattempts globally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conveniently, that is the default is we pass in None
Updated ✅
.tree() | ||
.simulate(Execution::StateReadOnly) | ||
.await | ||
.map_err(ChainCommunicationError::from_other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from_other_str
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.count() | ||
.simulate(Execution::StateReadOnly) | ||
.await | ||
.map_err(ChainCommunicationError::from_other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -81,6 +82,20 @@ impl Settings { | |||
.ok_or_else(|| eyre!("No chain setup found for {domain}")) | |||
} | |||
|
|||
/// Check and warn if reorg period is set for FuelVM domains. | |||
pub fn check_fuel_reorg(&self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why you need this? we can just ignore the param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was suggested to be added by @daniel-savu, the param is ignored, just adds a warning that the config has an invalid value
pub async fn new( | ||
conf: &ConnectionConf, | ||
locator: ContractLocator<'_>, | ||
mut wallet: WalletUnlocked, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wallet (readonly) seems sufficient in a lot of these places or is WalletUnlocked just for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency mostly, since we need an unlocked wallet anyway, it's cleaner to manage only one instance instead of using one for reads and one for writes
Description
Support FuelVM chains on the Hyperlane Protocol
Configurations for FuelTestnet and FuelIgnition
Backward compatibility
Yes
Testing
Contract repo E2E testing on local Fuel and EVM nodes
Contract repo E2E testing on Fuel Testnet and Base Sepolia
Notes
The configurations for the Fuel Testnet and Ignition are still WIP