-
Notifications
You must be signed in to change notification settings - Fork 55
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: support deterministic proxy contract deployment transaction (#2287) #2393
Conversation
1265fcf
to
ef15012
Compare
…action Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
cf0456e
to
539e674
Compare
packages/relay/src/lib/constants.ts
Outdated
@@ -175,4 +175,10 @@ export default { | |||
NEW_HEADS: 'newHeads', | |||
NEW_PENDING_TRANSACTIONS: 'newPendingTransactions', | |||
}, | |||
|
|||
// @source: Foundry related constants below can be found at https://github.com/Arachnid/deterministic-deployment-proxy?tab=readme-ov-file#latest-outputs |
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 think we want to remove references to "Foundry" as this can be relevant to others.
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.
Ah thanks for the good catch! Hmm I did try to make sure that I remove all the references to Foundry but welp missing that one! Pushing the fix in soon!
); | ||
const result = precheck.gasPrice( | ||
parsedDeterministicDeploymentTransaction, | ||
100 * constants.TINYBAR_TO_WEIBAR_COEF, |
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.
would this test be more meaningful of the gasPrice were a low number like 0 or 1 tinyBar?
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.
Hmm my understanding for this case is this. The 100 * constants.TINYBAR_TO_WEIBAR_COEF
(100 hbars) arg right there is the minimum gasPrice to compared against the parsedDeterministicDeploymentTransaction.gasPrice
(10 hbars).
The comparing condition is
const passes = txGasPrice >= minGasPrice || Precheck.isDeterministicDeploymentTransaction(tx);
in which, txGasPrice = parsedDeterministicDeploymentTransaction.gasPrice
(10 hbars), and minGasPrice = 100 * constants.TINYBAR_TO_WEIBAR_COEF
(100 hbars).
So if the minGasPrice is set to 0 or 1, then the condition txGasPrice >= minGasPrice
(10 >= 1) will be true, and regardless the outcome of the Precheck.isDeterministicDeploymentTransaction(tx);
this passes
will be true.
But if the minGasPrice is set to be greater than 10, then the condition txGasPrice >= minGasPrice
(10 >= 100) will be false -> now this condition will rely on the outcome of the Precheck.isDeterministicDeploymentTransaction(tx);
function -> and because it's the deterministic deployment the outcome will be true -> the passes
will be true.
More, isn't the whole purpose of this feature is to bypass deterministic deployment when its gasPrice (10hbar) is too low compared to network's gasPrice (minimum is 71 hbars for localnet)?
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.
Ah makes sense. I was incorrect in the way that the parameter was used.
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.
Looks good. Just a comment or two
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Quality Gate passedIssues Measures |
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.
LGTM, just resolve @lukelee-sl suggestions before merging.
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.
LGTM
) (#2393) * feat: supported foundry deterministic deployment proxy contract transaction Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * fix: removed Foundry reference Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> --------- Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
) (#2393) * feat: supported foundry deterministic deployment proxy contract transaction Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * fix: removed Foundry reference Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> --------- Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
) (#2393) * feat: supported foundry deterministic deployment proxy contract transaction Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * fix: removed Foundry reference Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> --------- Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
) (#2393) * feat: supported foundry deterministic deployment proxy contract transaction Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * fix: removed Foundry reference Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> --------- Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
* test: broke websocket acceptancetest CI task into smaller separate batches (#2382) (#2401) test: divided ws-server total acceptancetest into batches (#2382) Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * feat: support deterministic proxy contract deployment transaction (#2287) (#2393) * feat: supported foundry deterministic deployment proxy contract transaction Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * fix: removed Foundry reference Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> --------- Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * pkg: bumped hedera-local to 2.23.0 Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> --------- Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Description:
This PR adds support for the deterministic proxy contract deployment transaction by incorporating a utility function capable of identifying whether a signed transaction corresponds to the deterministic proxy contract deployment transaction. Upon recognition of such a transaction, it will skip the gasPrice precheck and proceed with submission to the network. In addition to this feature, unit tests, integration and e2e tests have been included to ensure the correct behavior.
*Notice: Currently, the
hedera-local
package is at its most recent version and exclusively supports theNETWORK_NODE_IMAGE_TAG
up to0.49.0-alpha.3
. However, thedeterministic proxy contract deployment transaction
feature is supported in theservices
at tag0.49.0-alpha.5
. Consequently, ABI Batch 1 and Websocket Batch 2 CI tasks are configured withnetworkTag
set to0.49.1
, which is the latest official services version encompassing0.49.0-alpha.5
. Nonetheless, in the near future, whenhedera-local
updates theNETWORK_NODE_IMAGE_TAG
to a version supporting thedeterministic deployment transaction
, these configurations will be deprecated and removed. This ticket is established to monitor this transition.Related issue(s):
Fixes #2287
Notes for reviewer:
Checklist