Add getAllTokens wasm binding to gui#1924
Conversation
|
""" WalkthroughA new public method Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend (JS)
participant DotrainOrderGui (Rust/WASM)
participant OrderbookYaml
participant TokenCfg
Frontend->>DotrainOrderGui: getAllTokens()
DotrainOrderGui->>OrderbookYaml: get_tokens()
OrderbookYaml->>TokenCfg: parse_all_from_yaml()
TokenCfg-->>OrderbookYaml: HashMap of tokens
OrderbookYaml-->>DotrainOrderGui: HashMap of tokens
DotrainOrderGui->>DotrainOrderGui: Filter tokens by current network
DotrainOrderGui->>DotrainOrderGui: Fetch missing metadata via ERC20 contract calls (async)
DotrainOrderGui-->>Frontend: Vec<TokenInfo> (filtered and enriched tokens)
Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code Graph Analysis (1)crates/settings/src/yaml/orderbook.rs (1)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
crates/js_api/src/gui/mod.rs(1 hunks)crates/js_api/src/gui/select_tokens.rs(2 hunks)crates/js_api/src/gui/state_management.rs(1 hunks)crates/settings/src/yaml/orderbook.rs(1 hunks)packages/orderbook/test/js_api/gui.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
crates/settings/src/yaml/orderbook.rs (1)
crates/settings/src/scenario.rs (1)
parse_all_from_yaml(352-402)
crates/js_api/src/gui/select_tokens.rs (2)
crates/js_api/src/gui/mod.rs (1)
initialize_gui_with_select_tokens(645-653)crates/common/src/erc20.rs (3)
decimals(51-67)name(69-85)symbol(87-103)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Deploy-Preview
- GitHub Check: git-clean
🔇 Additional comments (1)
crates/js_api/src/gui/mod.rs (1)
554-558: Extra network looks good – verify it’s used somewhereThe added
other-networkdefinition is syntactically correct and mirrors the existingsome-networksection.
Just double-check that:• No token/deployer/order accidentally still points at a non-existent
other-network,
• Tests exercise both networks (so the addition is not dead config).Otherwise this change is 👍.
|
|
||
| it('should get all tokens for current network', async () => { | ||
| mockServer | ||
| .forPost('/rpc-url') | ||
| .once() | ||
| .withBodyIncluding('0x82ad56cb') | ||
| .thenSendJsonRpcResult( | ||
| '0x00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000e000000000000000000000000000000000000000000000000000000000000001a0000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000007546f6b656e203100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000060000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000025431000000000000000000000000000000000000000000000000000000000000' | ||
| ); | ||
| mockServer | ||
| .forPost('/rpc-url') | ||
| .once() | ||
| .withBodyIncluding('0x82ad56cb') | ||
| .thenSendJsonRpcResult( | ||
| '0x00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000e000000000000000000000000000000000000000000000000000000000000001a000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000120000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000754656b656e203200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000060000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000025432000000000000000000000000000000000000000000000000000000000000' | ||
| ); | ||
|
|
||
| await gui.saveSelectToken('token1', '0x6666666666666666666666666666666666666666'); | ||
| await gui.saveSelectToken('token2', '0x8888888888888888888888888888888888888888'); | ||
|
|
||
| const allTokens = extractWasmEncodedData<TokenInfo[]>(gui.getAllTokens()); | ||
| assert.equal(allTokens.length, 2); | ||
| assert.equal(allTokens[0].address, '0x6666666666666666666666666666666666666666'); | ||
| assert.equal(allTokens[0].name, 'Token 1'); | ||
| assert.equal(allTokens[0].symbol, 'T1'); | ||
| assert.equal(allTokens[0].decimals, 6); | ||
| assert.equal(allTokens[1].address, '0x8888888888888888888888888888888888888888'); | ||
| assert.equal(allTokens[1].name, 'Teken 2'); | ||
| assert.equal(allTokens[1].symbol, 'T2'); | ||
| assert.equal(allTokens[1].decimals, 18); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid order-sensitive assertions & extract common RPC-mock helper
The new test hard-codes the index (allTokens[0], allTokens[1]) when asserting over the result of gui.getAllTokens().
getAllTokens() ultimately consumes a HashMap on the Rust side, so the order of the returned Vec depends on hash iteration order, which is not deterministic across targets or runs.
A flaky ordering would surface only in CI with different JS engines / seeds.
Recommended fix:
-assert.equal(allTokens.length, 2);
-assert.equal(allTokens[0].address, '0x6666666666666666666666666666666666666666');
-assert.equal(allTokens[0].name, 'Token 1');
-assert.equal(allTokens[0].symbol, 'T1');
-assert.equal(allTokens[0].decimals, 6);
-assert.equal(allTokens[1].address, '0x8888888888888888888888888888888888888888');
-assert.equal(allTokens[1].name, 'Teken 2');
-assert.equal(allTokens[1].symbol, 'T2');
-assert.equal(allTokens[1].decimals, 18);
+expect(allTokens).toHaveLength(2);
+
+const byAddr = Object.fromEntries(allTokens.map(t => [t.address.toLowerCase(), t]));
+
+expect(byAddr['0x6666666666666666666666666666666666666666']).toMatchObject({
+ name: 'Token 1',
+ symbol: 'T1',
+ decimals: 6,
+});
+expect(byAddr['0x8888888888888888888888888888888888888888']).toMatchObject({
+ name: 'Teken 2',
+ symbol: 'T2',
+ decimals: 18,
+});Nit: the RPC-mock boilerplate duplicated here is identical to earlier “select tokens” tests. Extracting a small helper (e.g. mockTokenInfo(name, symbol, decimals)) keeps tests shorter and easier to maintain.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should get all tokens for current network', async () => { | |
| mockServer | |
| .forPost('/rpc-url') | |
| .once() | |
| .withBodyIncluding('0x82ad56cb') | |
| .thenSendJsonRpcResult( | |
| '0x00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000e000000000000000000000000000000000000000000000000000000000000001a0000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000007546f6b656e203100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000060000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000025431000000000000000000000000000000000000000000000000000000000000' | |
| ); | |
| mockServer | |
| .forPost('/rpc-url') | |
| .once() | |
| .withBodyIncluding('0x82ad56cb') | |
| .thenSendJsonRpcResult( | |
| '0x00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000e000000000000000000000000000000000000000000000000000000000000001a000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000120000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000754656b656e203200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000060000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000025432000000000000000000000000000000000000000000000000000000000000' | |
| ); | |
| await gui.saveSelectToken('token1', '0x6666666666666666666666666666666666666666'); | |
| await gui.saveSelectToken('token2', '0x8888888888888888888888888888888888888888'); | |
| const allTokens = extractWasmEncodedData<TokenInfo[]>(gui.getAllTokens()); | |
| assert.equal(allTokens.length, 2); | |
| assert.equal(allTokens[0].address, '0x6666666666666666666666666666666666666666'); | |
| assert.equal(allTokens[0].name, 'Token 1'); | |
| assert.equal(allTokens[0].symbol, 'T1'); | |
| assert.equal(allTokens[0].decimals, 6); | |
| assert.equal(allTokens[1].address, '0x8888888888888888888888888888888888888888'); | |
| assert.equal(allTokens[1].name, 'Teken 2'); | |
| assert.equal(allTokens[1].symbol, 'T2'); | |
| assert.equal(allTokens[1].decimals, 18); | |
| }); | |
| // … earlier in the test … | |
| const allTokens = extractWasmEncodedData<TokenInfo[]>(gui.getAllTokens()); | |
| expect(allTokens).toHaveLength(2); | |
| const byAddr = Object.fromEntries( | |
| allTokens.map(t => [t.address.toLowerCase(), t]) | |
| ); | |
| expect(byAddr['0x6666666666666666666666666666666666666666']).toMatchObject({ | |
| name: 'Token 1', | |
| symbol: 'T1', | |
| decimals: 6, | |
| }); | |
| expect(byAddr['0x8888888888888888888888888888888888888888']).toMatchObject({ | |
| name: 'Teken 2', | |
| symbol: 'T2', | |
| decimals: 18, | |
| }); | |
| // … rest of the test … |
🤖 Prompt for AI Agents
In packages/orderbook/test/js_api/gui.test.ts around lines 1967 to 1997, the
test asserts token properties by fixed array indices which is unreliable due to
non-deterministic ordering from the underlying Rust HashMap. To fix this, modify
the test to assert token presence and properties without relying on order, for
example by searching tokens by address or name. Additionally, refactor the
duplicated RPC mock setup into a reusable helper function to reduce boilerplate
and improve test maintainability.
There was a problem hiding this comment.
no need as this is a one time thing
There was a problem hiding this comment.
@findolor Understood—I’ll leave the current assertions as-is. If you run into any ordering-related flakes later, feel free to revisit, but totally fine to proceed for this one-off case.
(つ﹏<)
| decimals: token.decimals.unwrap_or(18), | ||
| name: token.label.unwrap_or_else(|| token.key.clone()), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Magic constant ‘18’ – expose as a named constant.
Hard-coding the default ERC-20 decimals makes future changes/error-searching harder.
Declare once (e.g. const DEFAULT_DECIMALS: u8 = 18;) and reuse here and elsewhere for clarity.
🤖 Prompt for AI Agents
In crates/js_api/src/gui/select_tokens.rs at lines 147-148, replace the
hard-coded default decimal value 18 with a named constant. Declare a constant
like DEFAULT_DECIMALS of type u8 with the value 18 at the top of the file or an
appropriate module scope, then use this constant in place of the literal 18 in
the decimals assignment to improve code clarity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
crates/settings/src/yaml/orderbook.rs (1)
125-128: 🧹 Nitpick (assertive)Method looks correct but consider a doc-comment for discoverability
get_tokensmirrorsget_token_keysand returns the parsedHashMap<String, TokenCfg>.
A one-liner///comment would make IDE hover & generated docs clearer without adding maintenance burden.+ /// Return a map of all token configs (local + remote) keyed by YAML token keys. pub fn get_tokens(&self) -> Result<HashMap<String, TokenCfg>, YamlError> {packages/orderbook/test/js_api/gui.test.ts (1)
1967-1997: 🛠️ Refactor suggestionFlaky test ‒ avoid assuming deterministic order from
getAllTokens()
getAllTokens()wraps a RustHashMap → Vec, so element order is undefined across runs/targets.
Hard-codingallTokens[0],allTokens[1]will intermittently fail CI (see previous review).Refactor to assert set membership instead:
-const allTokens = extractWasmEncodedData<TokenInfo[]>(await gui.getAllTokens()); -assert.equal(allTokens.length, 2); -assert.equal(allTokens[0].address, '0x8888888888888888888888888888888888888888'); -assert.equal(allTokens[0].name, 'Teken 2'); -assert.equal(allTokens[0].symbol, 'T2'); -assert.equal(allTokens[0].decimals, 18); -assert.equal(allTokens[1].address, '0x6666666666666666666666666666666666666666'); -assert.equal(allTokens[1].name, 'Token 1'); -assert.equal(allTokens[1].symbol, 'T1'); -assert.equal(allTokens[1].decimals, 6); +const allTokens = extractWasmEncodedData<TokenInfo[]>(await gui.getAllTokens()); +expect(allTokens).toHaveLength(2); + +const byAddr = Object.fromEntries(allTokens.map(t => [t.address.toLowerCase(), t])); + +expect(byAddr['0x8888888888888888888888888888888888888888']).toMatchObject({ + name: 'Teken 2', + symbol: 'T2', + decimals: 18, +}); +expect(byAddr['0x6666666666666666666666666666666666666666']).toMatchObject({ + name: 'Token 1', + symbol: 'T1', + decimals: 6, +});(Consider extracting the repeated RPC-mock boilerplate into a helper per earlier feedback.)
crates/js_api/src/gui/select_tokens.rs (1)
394-447: Tests still rely on element order – make assertions order-agnostic
This was called out in a previous review and the risk remains.
Even with deterministic sorting, adding a new token whose name sorts earlier will break these index-based assertions.Convert the vector to a
HashMap<Address, TokenInfo>(or sort inside the test) and assert on keyed look-ups instead of positional indices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/js_api/Cargo.toml(1 hunks)crates/js_api/src/gui/select_tokens.rs(2 hunks)crates/js_api/src/gui/state_management.rs(2 hunks)crates/settings/src/yaml/orderbook.rs(2 hunks)packages/orderbook/test/js_api/gui.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: Deploy-Preview
🔇 Additional comments (1)
crates/js_api/Cargo.toml (1)
18-18:Details
❌ Incorrect review comment
Verify that the new
futuresdependency is actually needed
futures = { workspace = true }bumps the compile graph and increases the final WASM size.
From the diff it seems onlycrates/js_api/src/gui/state_management.rsimports the crate, and that import is currently unused (see separate comment). If it stays unused you can drop this dependency altogether.Run the quick grep to confirm:
🏁 Script executed:
#!/bin/bash # list Rust files that reference the `futures` crate rg --line-number '\bfutures::' -g '*.rs'Length of output: 445
Keep the workspace
futuresdependencyA quick grep shows the
futurescrate is actively used in multiple workspace crates, not justcrates/js_api:
crates/subgraph/src/multi_orderbook_client.rs:use futures::future::join_all;crates/js_api/src/gui/select_tokens.rs:.try_join_all(fetch_futures).await?crates/common/src/unit_tests/mod.rs:use futures::TryFutureExt;crates/common/src/fuzz/impls.rs:use futures::TryFutureExt;crates/common/src/dotrain_order/mod.rs:use futures::future::join_all;Since multiple components rely on
futures, the workspace dependency should remain.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/js_api/src/gui/state_management.rs (1)
287-287: Hard-coded base64 test fixture updated – same maintainability caveatThis line merely swaps one opaque blob for another.
Previous reviews already covered why the approach is brittle; no new technical issues introduced here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/js_api/src/gui/select_tokens.rs(3 hunks)crates/js_api/src/gui/state_management.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-tauri (ubuntu-22.04, true)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/js_api/src/gui/select_tokens.rs (1)
181-185: Repeatedto_lowercase()allocation – previously flaggedThe comparator still allocates on every comparison (
to_lowercase()).
Refer to the earlier review (commit e38f97f) for alternatives such as caching
orunicase::UniCase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/js_api/src/gui/select_tokens.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: git-clean
| let mut fetch_futures = Vec::new(); | ||
| let mut results = Vec::new(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Pre-allocate vectors & lift the magic concurrency constant
fetch_futures and results will never grow beyond tokens.len(), yet they
start empty and re-allocate as they grow.
Likewise 5 in .buffered(5) is an unexplained magic number.
- let mut fetch_futures = Vec::new();
- let mut results = Vec::new();
+ let capacity = tokens.len();
+ let mut fetch_futures = Vec::with_capacity(capacity);
+ let mut results = Vec::with_capacity(capacity);
+
+ const MAX_IN_FLIGHT_FETCHES: usize = 5; // tune to tasteand later
- .buffered(5)
+ .buffered(MAX_IN_FLIGHT_FETCHES)This removes unnecessary reallocations and documents the throttling policy in a
single place.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut fetch_futures = Vec::new(); | |
| let mut results = Vec::new(); | |
| // … earlier code … | |
| // pre-allocate to avoid repeated reallocations | |
| let capacity = tokens.len(); | |
| const MAX_IN_FLIGHT_FETCHES: usize = 5; // tune to taste | |
| let mut fetch_futures = Vec::with_capacity(capacity); | |
| let mut results = Vec::with_capacity(capacity); | |
| // build up your fetch_futures… | |
| for (_, token) in tokens { | |
| fetch_futures.push(fetch_token_info(token)); | |
| } | |
| // drive the in-flight requests, capping concurrency | |
| let fetched_results: Vec<TokenInfo> = futures::stream::iter(fetch_futures) | |
| .buffered(MAX_IN_FLIGHT_FETCHES) | |
| .try_collect::<Vec<_>>() | |
| .await?; | |
| // … later code … |
🤖 Prompt for AI Agents
In crates/js_api/src/gui/select_tokens.rs around lines 148 to 150, pre-allocate
the vectors fetch_futures and results with capacity tokens.len() to avoid
unnecessary reallocations. Also, replace the magic number 5 in the .buffered(5)
call with a named constant defined at the top of the file to document the
concurrency limit clearly and maintain it in a single place.
There was a problem hiding this comment.
no need to do this
There was a problem hiding this comment.
@findolor Understood—thanks for clarifying. I’ll leave the current implementation as-is.
There was a problem hiding this comment.
a constant instead of a magic number would be good. otherwise you need to know what buffer_unordered takes to understand what 5 represents
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| let mut fetch_futures = Vec::new(); | ||
| let mut results = Vec::new(); | ||
|
|
There was a problem hiding this comment.
a constant instead of a magic number would be good. otherwise you need to know what buffer_unordered takes to understand what 5 represents
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/js_api/src/gui/select_tokens.rs (1)
431-455: Tests still assume deterministic element orderThe new
test_get_all_tokensonce again usestokens[0],tokens[1], … even thoughget_all_tokens()only guarantees a stable order byname+address, not a specific position for any given token. Any naming change (or identical names) will cause spurious failures.Please make the assertions order-agnostic (e.g. collect into a
HashMapkeyed byaddress, or sort the test data before asserting).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/js_api/src/gui/select_tokens.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: Deploy-Preview
- GitHub Check: git-clean
- GitHub Check: test
| if token.decimals.is_none() || token.label.is_none() || token.symbol.is_none() { | ||
| let erc20 = ERC20::new(network.rpc.clone(), token.address); | ||
| fetch_futures.push(async move { | ||
| let token_info = erc20.token_info(None).await?; |
There was a problem hiding this comment.
hm what if there's 500 tokens in the list? the rpc will for sure ratelimit - do we need token decimals for the purposes of what we're doing?
There was a problem hiding this comment.
when we use a remote token link, we expect all the information for a token to be inside the response, meaning the decimal, symbol and label information is always present in the yaml file after fetching the remote tokens. this is the same case with entering a custom token using select token.
the only time these information might be missing is when the user is writing the token map in yaml manually. i doubt that they will input a lot of tokens there since we are giving them tools to write these better and more efficient. on the other hand i understand your point but i don't think this will be an issue for us
as for your question the token decimals are used for approval calldata generation
There was a problem hiding this comment.
Hmm, could we at least leave an option to not fetch them eagerly - if they're needed for approvals can't we just fetch them then if still missing?
There was a problem hiding this comment.
besides decimals, one other part we need the token information is to show the token name and symbol in the dropdown ui. we can fetch decimals during calldata generation but without the token info that shows up on the ui, the dropdown feature does not make sense
There was a problem hiding this comment.
to not get rate limited, we limit the amount of requests we send by the way. it's 5 requests max in flight
There was a problem hiding this comment.
we definitely need the token name and symbol for the ui, but that's usually there in the tokenlist json right?
There was a problem hiding this comment.
yeah like i said if there are missing fields in the response in each token data when we are fetching remote tokens from given url, an error is thrown saying that the response could not be decoded. this means it means the tokens we get are filled with the data we need
this missing data is only possible during manual typing
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
crates/js_api/src/gui/state_management.rs (1)
421-421: Hard-coded state blob updated again (same previous discussion)Just noting that the Base64 blob was refreshed – no functional concerns beyond the brittleness already covered in earlier reviews.
crates/js_api/src/gui/select_tokens.rs (3)
603-627: Tests still rely on deterministic ordering ofget_all_tokens()
The assertions index intotokens[0],tokens[1], … even though ordering depends on case-insensitive name sort and may change when names collide or new tokens are added.Recommend asserting via a
HashMap<Address, TokenInfo>or sorting in the test itself to avoid brittle failures.
350-353: One flaky RPC call nukes the whole result set
try_collectaborts on the firstErr, returning an error for all tokens.
Consider collecting successes and merely logging failures so the GUI degrades gracefully:- .buffer_unordered(MAX_CONCURRENT_FETCHES) - .try_collect() - .await?; + .buffer_unordered(MAX_CONCURRENT_FETCHES) + .filter_map(|res| async { + match res { + Ok(info) => Some(info), + Err(e) => { + log::warn!("Skipping token fetch: {e}"); + None + } + } + }) + .collect() + .await;This returns partial data instead of hard-failing in high-latency or rate-limited environments.
325-339: Preserve YAML-provided fields when only some metadata is missingThe current branch fetches on-chain metadata and then unconditionally overwrites every field, even those that were already present in YAML.
A user who manually specifieddecimals: 6but omittedlabelwill silently have their value replaced.- Ok::<TokenInfo, GuiError>(TokenInfo { - address: token.address, - decimals: token_info.decimals, - name: token_info.name, - symbol: token_info.symbol, - }) + Ok::<TokenInfo, GuiError>(TokenInfo { + address: token.address, + decimals: token.decimals.unwrap_or(token_info.decimals), + name: token.label.unwrap_or(token_info.name), + symbol: token.symbol.unwrap_or(token_info.symbol), + })This keeps existing YAML data authoritative while still filling gaps from the network.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
crates/js_api/src/gui/mod.rs(1 hunks)crates/js_api/src/gui/select_tokens.rs(3 hunks)crates/js_api/src/gui/state_management.rs(1 hunks)crates/settings/src/yaml/orderbook.rs(2 hunks)
🔇 Additional comments (2)
crates/js_api/src/gui/mod.rs (1)
925-929: Additional test network looks fineAdding
other-networkto the YAML fixture is self-contained and does not affect existing logic. 👍crates/js_api/src/gui/select_tokens.rs (1)
9-10: 👏 Replacing the magic literal withMAX_CONCURRENT_FETCHESimproves clarityUsing a named constant documents intent and removes the magic number.
No further action required.
3 participants
Motivation
See issue: #1600
This new function will allow us to fetch all the top level tokens from the yaml. This includes the tokens already available in the yaml, the remote tokens that are fetched from the network and the select tokens that are added.
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
fix #1600
Summary by CodeRabbit