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

Uniswap: summary of initialization of configuration #30

Merged
merged 9 commits into from
Sep 12, 2024

Conversation

mariaKt
Copy link
Contributor

@mariaKt mariaKt commented Sep 11, 2024

This PR includes the following changes:

  • the summarized rule that replaces the initial configuration with the one for UniswapV2SwapRenamed, if we identify the input as UniswapV2SwapRenamed.
  • a configuration variable that denotes whether the compiled input is the UniswapV2SwapRenamed test. It is currently boolean, and can be changed to an enum when we have more inputs to identify. If true, we apply the the summarized rule.
  • update to krun-sol to read the new configuration variable
  • new input files for the configuration variable all tests, true/false as needed

The mechanism used to apply the summarized rule is matching the contents of the new cell, to apply either the summarized rule or the initial rewrite rule

rule _:PragmaDefinition Ss:SourceUnits => Ss

, because for some reason I cannot figure out the priority does not help here.

@mariaKt mariaKt marked this pull request as ready for review September 11, 2024 17:31
Copy link
Contributor

@dwightguth dwightguth left a comment

Choose a reason for hiding this comment

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

the semantics itself looks fine, but I'd like us to tweak the test suite so that it's a little less messy to add the additional parameter.

bin/krun-sol Outdated
shift; shift
krun -d "$(dirname "$0")/../solidity-kompiled" "$contract" -cTXN="$txn" -pTXN="$(dirname "$0")/kparse-txn" --no-expand-macros "$@"
isuniswap="$3"
shift; shift; shift
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make krun-sol optionally take a third argument rather than making it required, and let's make the tests in test/regression automatically not pass that optional value, rather than adding a bunch of additional files to the test suite and breaking the interface of krun-sol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed. krun-sol has now an optional 3rd argument, that defaults to false. The tests in regression do not pass this argument.

@dwightguth dwightguth merged commit c01a219 into main Sep 12, 2024
1 check passed
@dwightguth dwightguth deleted the uniswap-init-summary branch September 12, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants