Fix/fee proxy legacy tx improvements #606

zees-dev · 2023-05-23T03:22:15Z

Context

The fee proxy precompile (callWithFeePreferences) is a bit flaky - due to inconsistencies in legacy and eip1559 tx types.
This PR updates the test suite and introduces handling of legacy tx type to handle the inconsistency and make the functionality more robust.

Changelog

Improvements to fee proxy functionality
E2E test suite for precompile improved
- Additional functions for legacy tx type (type 0/1)
- Calculating maxTokenAmount (instead of some high arbitrary value) - based on DEX token-pair liquidity
- Asserting the maxTokenAmount is always paid (this is the minimum payable - calculated from gasLimit and gasPrice (legacy tx) or maxFeePerGas (eip 1559 tx)
Updated rust code for get_fee_preferences_data - can also calculate TX cost(s) for legacy transactions
- Legacy transactions with low value supplied for maxTokenAmount hanged indefinitely (were not handled by pre-checks)
- Signature updated
- Fee proxy unit tests added (for gas price) & updated

Misc/unrelated

Updated some e2e tests in EVMGasCosts.test.ts - asserting total expected cost (robustness)
Fixed futurepass broken e2e test; due to the .only the futurepass precompile test suite was not running!

…e_per_gas (ignored)

surangap · 2023-05-24T23:37:09Z

pallet/fee-proxy/src/runner.rs

+		// Handle type 2 transactions (EIP1559)
+		let (max_fee_per_gas, max_priority_fee_per_gas) =
+			match (max_fee_per_gas, max_priority_fee_per_gas, is_transactional) {
+				// ignore priority fee, it becomes more expensive than legacy transactions


wonder why to ignore the priority fee here. shouldn't we account for it for type 2 correct gas calculations?

The calculation for total fee I had below resulted in too high of a gas cost if the max_priority_fee_per_gas was provided - to the point where it made no sense using the default EIP1559 tx type.

I have revisited the calculation now - and i think its incorrect. Hence will provide this in total fee calculation.

Looks good.
type 2 can result in a higher fee if the base_fee_per_gas is higher. our fee control pallet does not do base fee adjustments based on the network congestion as I could see.

After some deep debugging - there seem to be some issues with the max_priority_fee_per_gas; even for legacy tx's - some parts of the evm pallet/code seem to convert legacy to eip1559 tx; which cases the total_fee for the same transaction to be different. The max_priority_fee_per_gas introduces this difference.

Hence reverting back to the previous code - so that atleast for this function (fee proxy) there is alignment/equivalence between the 2 tx types.

…e for fee proxy

JasonTulp

Lgtm!

surangap · 2023-05-25T23:22:53Z

pallet/fee-proxy/src/tests.rs

+	}
+
+	#[test]
+	fn max_priority_fee_per_gas_ignored() {


nit- update the test name here

reverting the max_priority_fee_per_gas changes; a discrepancy appears - which causes the tests to fail :/

surangap · 2023-05-25T23:28:18Z

pallet/fee-proxy/src/tests.rs

+				Runner::calculate_total_gas(
+					gas_limit,
+					None,
+					Some(max_fee_per_gas),


Question - when a wallet is sending a type 2 tx, what is the max_fee_per_gas value here? should it be >= _base_fee? if they specify a value higher than _base_fee, should it charge a higher fee based on that and refund the extra?

thats the issue - refund does not occur since you've already paid in the non-native token (gas) - hence specifically for this function you'd need to lower the fees for eip1559 tx's

surangap

lgtm👍, a Couple of questions poped into my mind. added as comments

…iant release/2.35.0 (#595) Futurepass 1.5 (#588) * Add futurepass registrar precompile * Refactor futurepass precompile. * Link FuturePassRegistrarPrecompile and FuturePassPrecompileSet in the runtime * Rename createFuturepass bakc to create() in FuturePassRegistrarPrecompile * fmt * Add e2e tests for FuturepassRegistrarPrecompile * Update e2e tests for FuturepassPrecompile * Fix double charge of caller when sending value via proxyCall() * Existing tests all fixed * Update Futurepass precompile recursion to limit 1 * Fix precompile filter * Add new e2e tests * Fix linter * minor * Update FP_DELIGATE_RESERVE, FP_CREATION_RESERVE with descriptive comments * Remove redundant function isDelegate() from precompile * Update proxyCall() signature to accommodate value parameter. * Update precompile proxyCall() code to support value * Update precompile - value passed with payable is not considered inside the futurepass now. * Update existing e2e tests * add new e2e tests * Futurepass 1.5 ownable support (#589) * ownable iterface updates to align with open-zeppelin naming * introducing ownable interface to futurepass * transfer_futurepass function update, calls new remove_account fn in procy provider is new owner is None * updated benchmarking file fir transfer_futurepass signature upd * updated mocks with new remove_account in ProxyProvider trait * Futurepass precompile supports the Ownable interface * fixed typo * added test for transfer_futurepass_to_none_works * updated futurepass e2e tests, added tests for ownable * fix formatting * fixed ProxyProvider remove_account param types * upd futurepass precompile revert msg prefixes * fixed FP_DELIGATE_RESERVE typo -> FP_DELEGATE_RESERVE * commenting out function since functionality is not yet supported * commenting out and removing references to owner() function on fp precompile * removed redundant comment * fixed futurepass account removal unreserve logic * cargo fmt --all * fixed impl unreserve bug, allow death after transfer since account is removed * refactor tests, generic functions outside test suite * Add default receive() for futurepass precompile. * Futurepass precompile move Action::Default as the first. * Fix CI test build error * minor - use ref * fmt * minor * remove ErcIdConversion * Remove AddressMapping, refactor code. --------- Co-authored-by: zees-dev <63374656+zees-dev@users.noreply.github.com> Rename workflow name (#593) * Rename workflow name * Update workflow name * update workflow name based on suggestion Futurepass 1.5 improvements (#601) * proxy call create impl, proxy call emits Executed event * updating interfaces for futurepass - new events * upd futurepass precompile tests * Add futurepass precompile executed event data (#602) * Add Bytes32PostPad evm data type * Add get_selector() * write Executed data with Bytes32PostPad * Update tests * set amount as constant, added expect line to view exact value of fnCallData * move log cost record up --------- Co-authored-by: zees-dev <dev.786zshan@gmail.com> * create2 implementation * added tests for create2 * formatting * cargo fmt --all * create2 error fix * remove unused comment * comment update --------- Co-authored-by: surangap <19677661+surangap@users.noreply.github.com> Fix transfer bug (#596) * Fix transfer bug * update test comments * Fix broken test SFT Pallet (#505) * Init commit * Fix build errors * Add some details and remove tests * Feat/sft create collection (#506) * Add create sft collection extrinsic Change CollectionNameType to BoundedVec * Changes event to BoundedVec * Feat/sft fix mock (#512) * Add create sft collection extrinsic Change CollectionNameType to BoundedVec * Fix mock and add test template * Fix all tests, adds tests for create_collection * Change event to BoundedVec * Feat/sft create collection (#506) * Add create sft collection extrinsic Change CollectionNameType to BoundedVec * Changes event to BoundedVec * Frevert add token (#509) * First pass: add token * Errors * Slight changes * bit of pr feedback * Moved extrinsic logic to impls file, changed event * Changed name type to boundedVec --------- Co-authored-by: JasonTulp <jason.j.tulp@gmail.com> * Feat/sft mint extrinsic (#532) * Add mint extrinsic * Add tests for create_token Still needs more mint tests * Add more tests for mint extrinsic * Adds test for minting multiple serials * Rename mint_balance * SFT Transfer Extrinsic (#540) * Add mint extrinsic * Add tests for create_token Still needs more mint tests * Add more tests for mint extrinsic * Adds test for minting multiple serials * Rename mint_balance * Adds transfer extrinsic and tests * Adds more tests * Updated comment * Feat/set owner (#543) * Remove claim_unowned_collection * Set new collection owner * tests * comment * move transactionals * Sft set max issuance and set base uri (#544) * Adds set_max_issuance and set_base_uri extrinsics * Adds set_max_issuance and set_base_uri extrinsics * Move sanity checks * Feat/sft burn extrinsic (#542) * Add mint extrinsic * Add tests for create_token Still needs more mint tests * Add more tests for mint extrinsic * Adds test for minting multiple serials * Rename mint_balance * Adds transfer extrinsic and tests * Adds more tests * Adds burn extrinsic * Change serial_numbers type * SFT benchmarks (#547) * fix benchmarking errors * Add boilerplate benchmark code * Fixed benchmark tests * Run benchmark and add weights --------- Co-authored-by: Justin Frevert <justinfrevert@gmail.com> * Feat/sft benchmark verify (#549) * fix benchmarking errors * Add boilerplate benchmark code * Fixed benchmark tests * add verify line * more verify * Add more verify... * more verify * fix weird merge * More from bad merge * Update pallet/sft/src/benchmarking.rs Co-authored-by: JasonT <50426640+JasonTulp@users.noreply.github.com> * little pr things * remove extra line --------- Co-authored-by: JasonTulp <jason.j.tulp@gmail.com> Co-authored-by: JasonT <50426640+JasonTulp@users.noreply.github.com> * Chore/sft storage migration (#552) * Add NFT migration for CollectionInfo * Remove TODO * Address PR Comments * Fix trait bound issue in mock * Merge changes, still needs migration fix * Remove without_storage_info from SFT and NFT pallets * Adds NFT migration * Links up NFT migration and removes old migration * Adds migration for bridged collections * Fix tests * Fix comment * Try fix CI * Fix merge * Fix test * Updated current storage version. Replaced translate with iter/remove/insert * Rename event, add transfer check --------- Co-authored-by: justinFrevert <81839854+justinFrevert@users.noreply.github.com> Co-authored-by: Justin Frevert <justinfrevert@gmail.com> Co-authored-by: Marko Petrlic <petrlic.marko@gmail.com> Add xrpl proof signature verification tool. (#424) * Add xrpl proof signature verification tool. * fix typo * Use libsecp256k1 directly Fix/fee proxy legacy tx improvements (#606) * evm gas costs e2e tests - expected cost assertions * fee preferences, additional tests, suite made more robust * retrieving relevant params from different types of txs * updated runner - now takes into account gas_price and max_priority_fee_per_gas (ignored) * cargo fmt --all * fee preferences runner unit test updates * removed futurepass single test restriction * fixed broken futurepass test * call with fee preferences test for futurepass precompile * max_priority_fee_per_gas taken into account when calculating total fee for fee proxy * fee preferences test updated to not run futurepass tests * cargo fmt --all * reverted max_priority_fee_per_gas changes - this is ignored again [Feat] Dex uniswap adaptation (#607) * Implement recipient and deadline functionalities in dex pallet * Fix unit tests * Fix dex e2e test * Rename dex parameter names to align with uniswapv2 * Remove min_share_increment parameter in add_liquidity * Add extra tests * Adapt e2e tests * Rename the error message * Fix benchmarking errors * Fix dex e2e test * Unit test fix after review * Move deadline checks to underlying functions * Adapt do_* functions to preparefor uniswap precompile Add `curl` library to docker image (#614) Release/3.36.0 (#615) * release 3.26 .0 * forgot Cargo.lock * Remove old migrations * fix removals * re add missing part * corrections Fixes coverage workflow on new comment (#604) * Removes coverage workflow on new comment * Fixes coverage * Take 2 * Take 3 * Take 4 * Final take.. * Check to see if this workflow is even the one being run * Revert to favoured solution WIP Update read me As per review comment Cargo fmt Fix build Cargo fmt Convert serial number to vec Fix build Debug test Prettier format Use log4 for ERC721 approve and transfer events Update log4 for Transfer and Approval events for ERC721 Remove commented code

zees-dev added 6 commits May 23, 2023 15:14

evm gas costs e2e tests - expected cost assertions

08d1e54

fee preferences, additional tests, suite made more robust

6fba721

retrieving relevant params from different types of txs

d116770

updated runner - now takes into account gas_price and max_priority_fe…

c194518

…e_per_gas (ignored)

cargo fmt --all

27c928c

fee preferences runner unit test updates

fb98cde

zees-dev marked this pull request as ready for review May 23, 2023 03:36

zees-dev requested review from JasonTulp, justinfrevert, markopoloparadox and surangap May 23, 2023 03:37

zees-dev added 3 commits May 23, 2023 15:45

removed futurepass single test restriction

99e0bf3

fixed broken futurepass test

df9ca1c

call with fee preferences test for futurepass precompile

c752419

justinfrevert approved these changes May 24, 2023

View reviewed changes

markopoloparadox approved these changes May 24, 2023

View reviewed changes

surangap reviewed May 24, 2023

View reviewed changes

zees-dev added 3 commits May 26, 2023 00:26

max_priority_fee_per_gas taken into account when calculating total fe…

14afdb9

…e for fee proxy

fee preferences test updated to not run futurepass tests

c663371

cargo fmt --all

9538975

JasonTulp approved these changes May 25, 2023

View reviewed changes

surangap reviewed May 25, 2023

View reviewed changes

surangap approved these changes May 25, 2023

View reviewed changes

reverted max_priority_fee_per_gas changes - this is ignored again

cc49a06

zees-dev merged commit 556e633 into main May 26, 2023

zees-dev deleted the fix/fee-proxy-legacy-tx-improvements branch May 26, 2023 02:55

justinfrevert mentioned this pull request May 29, 2023

Release 3.36.0 #611

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix/fee proxy legacy tx improvements #606

Fix/fee proxy legacy tx improvements #606

Uh oh!

zees-dev commented May 23, 2023 •

edited

Loading

Uh oh!

surangap May 24, 2023

Uh oh!

zees-dev May 25, 2023

Uh oh!

surangap May 25, 2023 •

edited

Loading

Uh oh!

zees-dev May 25, 2023

Uh oh!

JasonTulp left a comment

Uh oh!

surangap May 25, 2023

Uh oh!

zees-dev May 25, 2023

Uh oh!

surangap May 25, 2023

Uh oh!

zees-dev May 25, 2023

Uh oh!

surangap left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Fix/fee proxy legacy tx improvements #606

Fix/fee proxy legacy tx improvements #606

Uh oh!

Conversation

zees-dev commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changelog

Misc/unrelated

Uh oh!

surangap May 24, 2023

Choose a reason for hiding this comment

Uh oh!

zees-dev May 25, 2023

Choose a reason for hiding this comment

Uh oh!

surangap May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zees-dev May 25, 2023

Choose a reason for hiding this comment

Uh oh!

JasonTulp left a comment

Choose a reason for hiding this comment

Uh oh!

surangap May 25, 2023

Choose a reason for hiding this comment

Uh oh!

zees-dev May 25, 2023

Choose a reason for hiding this comment

Uh oh!

surangap May 25, 2023

Choose a reason for hiding this comment

Uh oh!

zees-dev May 25, 2023

Choose a reason for hiding this comment

Uh oh!

surangap left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zees-dev commented May 23, 2023 •

edited

Loading

surangap May 25, 2023 •

edited

Loading