fix(wasm): add test-ext-api feature to mm2_main and mm2_bin_lib tomls by laruh · Pull Request #2295 · GLEECBTC/komodo-defi-framework

laruh · 2024-12-16T10:38:35Z

Fixes wasm build with feature flag test-ext-api

hound@hound:~/komodo-defi-framework$ docker run -v "$(pwd)":/app -w /app kdf-build-container wasm-pack build mm2src/mm2_bin_lib --target web --out-dir wasm_build/deps/pkg/ --features  test-ext-api
[INFO]: Checking for the Wasm target...
info: downloading component 'rust-std' for 'wasm32-unknown-unknown'
info: installing component 'rust-std' for 'wasm32-unknown-unknown'
[INFO]: Compiling to Wasm...
warning: /app/mm2src/mm2_bin_lib/Cargo.toml: file `/app/mm2src/mm2_bin_lib/src/mm2_bin.rs` found to be present in multiple build targets:
  * `bin` target `mm2`
  * `bin` target `kdf`
error: none of the selected packages contains these features: test-ext-api
Error: Compiling your crate to WebAssembly failed
Caused by: Compiling your crate to WebAssembly failed
Caused by: failed to execute `cargo build`: exited with exit status: 101
  full command: cd "mm2src/mm2_bin_lib" && "cargo" "build" "--lib" "--release" "--target" "wasm32-unknown-unknown" "--features" "test-ext-api"

also fixes CI PR lint, checking new status: labels

dimxy · 2024-12-16T11:18:06Z

I can see there are PR statuses like 'invalid', 'blocked'. Should we allow to run CI for those?

onur-ozkan · 2024-12-16T11:32:17Z

.github/workflows/pr-lint.yml

@@ -50,5 +50,15 @@ jobs:
          LABEL_NAMES: ${{ toJson(github.event.pull_request.labels.*.name) }}
        if: contains(env.LABEL_NAMES, 'under review') == contains(env.LABEL_NAMES, 'in progress')


This needs to be updated.

Forgot to remove

as we have 3rd label to check, I decided to use && 3678a8b

onur-ozkan · 2024-12-16T11:32:39Z

.github/workflows/pr-lint.yml

-          echo "PR must have "exactly one" of these labels: ['under review', 'in progress']."
-          exit 1
+          # Filter labels starting with "status:"
+          STATUS_LABELS=$(echo $LABEL_NAMES | jq -r '.[] | select(startswith("status:"))')
+          STATUS_COUNT=$(echo "$STATUS_LABELS" | wc -l)
+
+          echo "Found status labels: $STATUS_LABELS"
+          echo "Count of status labels: $STATUS_COUNT"
+
+          if [ "$STATUS_COUNT" -ne 1 ]
+          then
+            echo "PR must have exactly one label starting with 'status:'. Found $STATUS_COUNT."
+            exit 1
+          fi


We don't need this much change here I think

We just need to replace the old labels, I don't know why we changed the entire logic

We just need to replace the old labels, I don't know why we changed the entire logic

I decided that almost all status labels are valid and can pass the PR lint. Are you suggesting allowing only the 'pending review' and 'in progress' statuses to pass the lint check?

Label validation logic was simple: requiring one of "in progress" or "under review" labels but not both at the same time. We can update this logic with new ones + blocked label.

We can update this logic with new ones + blocked label.

That what I wanted to do, allow new status labels except invalid statuses, just without rewriting all statuses manually.
But I think now, its better to be explicit with labels in pr lint for the case if we add new status labels in the future

I didn't mean all new labels, what I mean is these 3 labels:

status: pending review

status: blocked

status: in progress

laruh · 2024-12-16T11:33:24Z

I can see there are PR statuses like 'invalid', 'blocked'. Should we allow to run CI for those?

hmm I see

dimxy · 2024-12-16T13:52:58Z

.github/workflows/pr-lint.yml

        env:
          LABEL_NAMES: ${{ toJson(github.event.pull_request.labels.*.name) }}
-        if: contains(env.LABEL_NAMES, 'under review') == contains(env.LABEL_NAMES, 'in progress')
+        if: "!((contains(env.LABEL_NAMES, 'pending review') && !contains(env.LABEL_NAMES, 'in progress') && !contains(env.LABEL_NAMES, 'blocked'))


maybe you could use '!=' as 'xor' ?

if: ! (contains(env.LABEL_NAMES, 'pending review') != contains(env.LABEL_NAMES, 'in progress') != contains(env.LABEL_NAMES, 'blocked')) ... exit 1

xor would treat true, true, true the same as true, false, false. that said im not sure how an expression with multiple == or != execute (they always confuse me so i just pretend they never existed xD) so you are using xor here as an analogy or they actually have the very same effect.
solutions i could think of:

collect these results in a list and count how many trues we have.

add up the contains result and make sure they are equal to 1, booleans should support loose type coercion (false -> 0, true -> 1): https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#operators

Yes 'xor' would not work. Looks like there is no '+' operator either.
I guess we need to set env vars to sum them (like it was done in one of the recent commits)

oh. indeed + isn't listed in the op list.

maybe we can just avoid github actions' if clause and write the complete logic inside the run: in good old bash instead.

@dimxy @mariocynicys

I suggest to move to bash if we decide to make lint logic more complex.
bash is more flexible, but less readable with more code lines required.

If we all agree with current labels logic:

Allow any number of labels, but only one of the following labels must be present at a time: ['status: pending review', 'status: in progress', 'status: blocked']. These labels cannot be set together.

then I suggest to leave GH actions expression.

bash is more flexible, but less readable with more code lines required.

i would argue that the current check is harder to read than if it was written in bash.

it would look like this

- name: Check PR labels env: LABEL_NAMES: ${{ toJson(github.event.pull_request.labels.*.name) }} run: | # Convert LABEL_NAMES (JSON) to a string LABEL_NAMES_STR=$(echo "${LABEL_NAMES}" | jq -r '.[]') # Define allowed labels ALLOWED_LABELS=("status: pending review" "status: in progress" "status: blocked") MATCHED_COUNT=0 # Check for matches in LABEL_NAMES for label in "${ALLOWED_LABELS[@]}"; do if echo "$LABEL_NAMES_STR" | grep -Fxq "$label"; then ((MATCHED_COUNT++)) fi done # Validate exactly one match if [[ "$MATCHED_COUNT" -ne 1 ]]; then echo "PR must have exactly one of these labels: ['status: pending review', 'status: in progress', 'status: blocked']." exit 1 fi

this is too much, github actions allow to do the same in a more light weight way right in if block without waisting execution time on run

if we need to just be sure that labels have only one of 3 necessary labels this is simpler

- name: Check PR labels env: LABEL_NAMES: ${{ toJson(github.event.pull_request.labels.*.name) }} if: "!((contains(env.LABEL_NAMES, 'pending review') && !contains(env.LABEL_NAMES, 'in progress') && !contains(env.LABEL_NAMES, 'blocked')) || (!contains(env.LABEL_NAMES, 'pending review') && contains(env.LABEL_NAMES, 'in progress') && !contains(env.LABEL_NAMES, 'blocked')) || (!contains(env.LABEL_NAMES, 'pending review') && !contains(env.LABEL_NAMES, 'in progress') && contains(env.LABEL_NAMES, 'blocked')))" run: | echo "PR must have "exactly one" of these labels: ['status: pending review', 'status: in progress', 'status: blocked']." exit 1

Maybe we can do 2 separate checks:
we have only one PR label starting with "status:" (if not then error 'duplicate status')
PR labels contain any from the list of allowed labels.

we have only one PR label starting with "status:" (if not then error 'duplicate status')

I dont see problem of having status: pending review and status: pending contract deployment or status: pending infra etc together.
Only one status i doubt is status: invalid I would move it to PR must have "exactly one" of these labels list

PR labels contain any from the list of allowed labels.

I dont understand what do you mean, above you suggested to have only one status label

I dont understand what do you mean, above you suggested to have only one status label

I mean if we have already ensured that the PR has only single "status:" label,
it is easy to check that this label is one from allowed, like: contains(env.LABEL_NAMES, 'pending review') || contains(env.LABEL_NAMES, 'in progress') || contains(env.LABEL_NAMES, 'blocked')

I dont understand what do you mean, above you suggested to have only one status label

I mean if we have already ensured that the PR has only single "status:" label, it is easy to check that this label is one from allowed, like: contains(env.LABEL_NAMES, 'pending review') || contains(env.LABEL_NAMES, 'in progress') || contains(env.LABEL_NAMES, 'blocked')

these also will lead to move the whole logic to run block using bash, while we can do the same right in one if block using GH actions expressions and it will use run only in the case of false result.

- name: Determine label validity id: label_check env: LABEL_NAMES: ${{ toJson(github.event.pull_request.labels.*.name) }} run: | status_count=$(echo "$LABEL_NAMES" | jq '[.[] | select(startswith("status:"))] | length') allowed_status_count=$(echo "$LABEL_NAMES" | jq '[.[] | select(. == "status: pending review" or . == "status: in progress" or . == "status: blocked")] | length') if [ "$status_count" -eq 1 ] && [ "$allowed_status_count" -eq 1 ]; then # Valid scenario echo "valid=true" >> $GITHUB_OUTPUT else # Invalid scenario echo "valid=false" >> $GITHUB_OUTPUT fi - name: Check PR labels if: steps.label_check.outputs.valid == 'false' run: | echo "PR must have exactly one of these labels: ['status: pending review', 'status: in progress', 'status: blocked']." exit 1

dimxy · 2024-12-16T19:13:47Z

I am testing 1inch api with wasm (maybe more fixes will be here)

dimxy · 2024-12-18T08:32:48Z

I am testing 1inch api with wasm (maybe more fixes will be here)

BTW no fixes for wasm is expected for now. We discovered that for wasm we can call 1inch API only via komodo-proxy (due to the CORS check by 1inch API)

onur-ozkan

I guess it could be nicer to write it in plain bash style but I don't mind that (it's not that critical). If it works, it's fine.

dimxy

LGTM

* dev: (35 commits) fix(crypto): allow non bip39 mnemonics storage (GLEECBTC#2312) fix(legacy_swap): check for existing maker/taker payment before timeout (GLEECBTC#2283) feat(tendermint): validators RPC (GLEECBTC#2310) chore(CI): validate Cargo lock file (GLEECBTC#2309) test(P2P): add test for peer time sync validation (GLEECBTC#2304) fix mm2_p2p dev build (GLEECBTC#2311) update Cargo.lock (GLEECBTC#2308) chore(CI): unlock wasm-pack version (GLEECBTC#2307) add `wasm` feature on WASM for timed-map (GLEECBTC#2306) replace broken rpc link (GLEECBTC#2305) chore(eth-websocket): remove some unnecessary wrappers (GLEECBTC#2291) improvement(CI): switch to proper rust caching (GLEECBTC#2303) fix(wasm): add test-ext-api feature to mm2_main and mm2_bin_lib tomls (GLEECBTC#2295) chore(ci): Update docker build for wasm (GLEECBTC#2294) chore(p2p): follow-up nits (GLEECBTC#2302) feat(p2p): ensure time synchronization in the network (GLEECBTC#2255) bump libp2p (GLEECBTC#2296) chore(adex-cli): use "Komodo DeFi Framework" name in adex_cli (GLEECBTC#2290) chore(ctx): replace gstuff constructible with oncelock (GLEECBTC#2267) don't rely on core (GLEECBTC#2289) ...

…#2295) * add feature flag to mm2_main and mm2_bin_lib tomls * filter 'status:' pr labels * remove old labels * dont allow to pass blocked and invalid statuses * pass in progress and pending review statuses * update statuses list

add feature flag to mm2_main and mm2_bin_lib tomls

0576c25

laruh added 2.4.0-beta status: pending review labels Dec 16, 2024

laruh requested a review from smk762 December 16, 2024 10:38

laruh changed the title ~~add feature flag to mm2_main and mm2_bin_lib tomls~~ fix(wasm): add test-ext-api feature to mm2_main and mm2_bin_lib tomls Dec 16, 2024

laruh requested a review from dimxy December 16, 2024 10:43

filter 'status:' pr labels

86f8312

onur-ozkan reviewed Dec 16, 2024

View reviewed changes

remove old labels

201bb32

smk762 previously approved these changes Dec 16, 2024

View reviewed changes

dont allow to pass blocked and invalid statuses

07b2f85

laruh dismissed smk762’s stale review via 07b2f85 December 16, 2024 11:52

laruh added status: blocked status: invalid and removed status: pending review status: blocked labels Dec 16, 2024

pass in progress and pending review statuses

456eb17

laruh added status: pending review status: in progress status: blocked and removed status: invalid status: pending review status: in progress status: blocked labels Dec 16, 2024

laruh force-pushed the dev-test-ext-api branch 2 times, most recently from 7fe6547 to 50e1e1e Compare December 16, 2024 13:23

laruh added status: pending review and removed status: blocked labels Dec 16, 2024

update statuses list

3678a8b

laruh force-pushed the dev-test-ext-api branch from 63d0a1a to 3678a8b Compare December 16, 2024 13:33

laruh added status: blocked status: pending review and removed status: pending review status: blocked labels Dec 16, 2024

dimxy reviewed Dec 16, 2024

View reviewed changes

laruh requested review from dimxy, mariocynicys and onur-ozkan December 23, 2024 13:01

onur-ozkan approved these changes Dec 25, 2024

View reviewed changes

onur-ozkan added status: pending review status: in progress and removed status: pending review 2.4.0-beta status: in progress labels Dec 25, 2024

dimxy approved these changes Dec 25, 2024

View reviewed changes

onur-ozkan merged commit c5f544c into dev Dec 25, 2024

onur-ozkan deleted the dev-test-ext-api branch December 25, 2024 08:09

shamardy mentioned this pull request Apr 16, 2025

chore(release): v2.4.0-beta #2345

Merged

		@@ -50,5 +50,15 @@ jobs:
		LABEL_NAMES: ${{ toJson(github.event.pull_request.labels.*.name) }}
		if: contains(env.LABEL_NAMES, 'under review') == contains(env.LABEL_NAMES, 'in progress')

Conversation

laruh commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimxy commented Dec 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laruh commented Dec 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laruh Dec 17, 2024 • edited by mariocynicys Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laruh Dec 17, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laruh Dec 18, 2024 • 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!

dimxy commented Dec 16, 2024

Uh oh!

dimxy commented Dec 18, 2024

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

dimxy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

laruh commented Dec 16, 2024 •

edited

Loading

laruh Dec 17, 2024 •

edited by mariocynicys

Loading

laruh Dec 17, 2024 •

edited

Loading

laruh Dec 18, 2024 •

edited

Loading