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

feat: SVM priority fee oracles and transaction submitters, more consistent tx submission #4959

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

tkporter
Copy link
Collaborator

@tkporter tkporter commented Dec 6, 2024

Description

The fruition of https://www.notion.so/hyperlanexyz/Landing-Solana-Transactions-15a6d35200d680cbaf5eea5f9dbec1b1

Major highlights:

Drive-by changes

Related issues

Backward compatibility

Testing

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.53%. Comparing base (fd20bb1) to head (356c107).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4959   +/-   ##
=======================================
  Coverage   77.53%   77.53%           
=======================================
  Files         103      103           
  Lines        2110     2110           
  Branches      190      190           
=======================================
  Hits         1636     1636           
  Misses        453      453           
  Partials       21       21           
Components Coverage Δ
core 87.80% <ø> (ø)
hooks 79.39% <ø> (ø)
isms 83.68% <ø> (ø)
token 91.27% <ø> (ø)
middlewares 79.80% <ø> (ø)

Copy link

changeset-bot bot commented Dec 6, 2024

⚠️ No Changeset found

Latest commit: 570c52b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes changesets to release 9 packages
Name Type
@hyperlane-xyz/cli Patch
@hyperlane-xyz/ccip-server Patch
@hyperlane-xyz/github-proxy Patch
@hyperlane-xyz/helloworld Patch
@hyperlane-xyz/infra Patch
@hyperlane-xyz/sdk Patch
@hyperlane-xyz/utils Patch
@hyperlane-xyz/widgets Patch
@hyperlane-xyz/core Patch

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@tkporter tkporter changed the title [wip] feat: Helius and constant priority fee oracles feat: SVM priority fee oracles and transaction submitters, more consistent tx submission Dec 13, 2024
@@ -785,7 +785,7 @@
"index": {
"from": 1,
"mode": "sequence",
"chunk": 100
"chunk": 20
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these are some drive-bys configs that were applied for operational reasons to the statefulsets but hadn't yet made it into the mainnet config

Copy link
Contributor

Choose a reason for hiding this comment

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

could you also update the metadata in registry pls so future config generation respects the values 🙏

}

/// Gets the estimated costs for a given instruction.
pub async fn get_estimated_costs_for_instruction(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

honestly not sure if these fns should live here, in provider.rs, or somewhere else - curious if anyone has strong takes

@@ -17,7 +17,9 @@ spec:
metadata:
annotations:
checksum/configmap: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }}
checksum/external-secret: {{ include (print $.Template.BasePath "/external-secret.yaml") . | sha256sum }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

drive-bys to make sure pods get updated if doing a release and there's a checksum change of the external secrets

@@ -501,7 +550,7 @@ const releaseCandidate: RootAgentConfig = {
rpcConsensusType: RpcConsensusType.Fallback,
docker: {
repo,
tag: '4cb2c9a-20241205-142854',
tag: 'b98678c-20241213-043725',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you can see this running in RC atm https://solscan.io/account/G6fWKEBSrDtR8Uzx4kdVRRhrsHZ6zmGX1G2TYMmMgK6u - though might run out of funds. I'd expect some reverts as it's competing with the hyperlane relayer

check out the fees!

}

// An ugly way to mark a URL as a the secret Helius URL when Helm templating
export const HELIUS_SECRET_URL_MARKER = 'helius';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

intent here is to let us easily say in code "I want the helius RPC URL here" and have external secrets replace it


// TODO: not sure if this actually checks if the transaction was executed / reverted?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

was poking into this and I think we may need to change this - iiuc TxOutcome.executed is supposed to be false if the tx reverted, but I think executed will be true just if the tx lands

@tkporter tkporter marked this pull request as ready for review December 13, 2024 05:23
Comment on lines +258 to +264
// If the protocol type is Sealevel, require everything in AgentSealevelChainMetadataSchema
if (metadata.protocol === ProtocolType.Sealevel) {
if (!AgentSealevelChainMetadataSchema.safeParse(metadata).success) {
return false;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

i can't see where these new params are in the agent config?

Comment on lines +2 to 5
'@hyperlane-xyz/cli': patch
---

Suppress help on CLI failures
Copy link
Contributor

Choose a reason for hiding this comment

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

need a new changeset for the SDK changes

@@ -0,0 +1,225 @@
#![allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove it if we add annotations directly to fields which are not used:

#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
struct JsonRpcResult<T> {
    #[allow(dead_code)]
    jsonrpc: String,
    #[allow(dead_code)]
    id: String,
    result: T,
}

@@ -0,0 +1,121 @@
#![allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove it if we remove this unused function:

pub fn new_with_rpc_client(rpc_client: SealevelRpcClient) -> Self {
        Self { rpc_client }
    }

Comment on lines +528 to +529
// The retuend costs are unused at the moment - we simply want to perform a simulation to
// determine if the message will revert or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The retuend costs are unused at the moment - we simply want to perform a simulation to
// determine if the message will revert or not.
// The retuned costs are unused at the moment - we simply want to perform a simulation to
// determine if the message will revert or not.

Comment on lines +260 to +261
if (!AgentSealevelChainMetadataSchema.safeParse(metadata).success) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: return AgentSealevelChainMetadataSchema.safeParse(metadata).success

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

4 participants