feat: Enhance smoke tests with management API functionality#45
Open
dylankilkenny wants to merge 1 commit intomainfrom
Open
feat: Enhance smoke tests with management API functionality#45dylankilkenny wants to merge 1 commit intomainfrom
dylankilkenny wants to merge 1 commit intomainfrom
Conversation
Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR enhances the smoke test script by adding comprehensive management API testing for fee limits CRUD operations, while also improving network-specific configuration handling.
Key Changes:
- Adds a new
mgmt-apitest that validates fee limits CRUD operations (get usage, get/set/delete limits) - Implements network-aware default values for RPC URLs and contract IDs (testnet vs mainnet)
- Updates the context type to include
apiKeyandadminSecretfor management API operations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| console.log(` ✓ ${label}: ${hash || status || 'confirmed'}`); | ||
| // For tx results, show hash; for mgmt results, show ok or just confirmed | ||
| const detail = hash || status || (ok !== undefined ? `ok=${ok}` : 'ok'); | ||
| console.log(` ${label}: ${detail}`); |
There was a problem hiding this comment.
The checkmark symbol (✓) has been removed from the success output. This changes the visual feedback users receive when tests pass successfully. The checkmark provided a clear, at-a-glance indication of success. Consider retaining the checkmark prefix for consistency with the original UX.
Suggested change
| console.log(` ${label}: ${detail}`); | |
| console.log(` ✓ ${label}: ${detail}`); |
| } else { | ||
| const error = envelope?.error || 'unknown error'; | ||
| console.log(` ✗ ${label}: ${error}`); | ||
| console.log(` ${label}: FAILED - ${error}`); |
There was a problem hiding this comment.
The error output format has been changed from "✗" to no symbol and from just the error to "FAILED - " prefix. While this might be intentional, the removal of the "✗" symbol makes it less visually distinct from success messages. Consider retaining the "✗" symbol for better visual distinction between success and failure cases.
Suggested change
| console.log(` ${label}: FAILED - ${error}`); | |
| console.log(` ${label}: ✗ FAILED - ${error}`); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/smoke.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
scripts/smoke.ts (5)
9-9: LGTM! Clear documentation of new features.The documentation updates accurately describe the new management API test and related CLI flags.
Also applies to: 55-55, 59-60
158-162: LGTM! Sensible network-aware defaults.The conditional RPC URL defaults based on network type improve usability and align with common Stellar endpoints.
211-214: LGTM! Context extension is appropriate for the new test.Adding
apiKeyandadminSecretto the execution context allows the management API test to access the necessary credentials while keeping the implementation clean.
418-440: LGTM! Improved result printing handles management API responses.The modified
printResultfunction now correctly handles both transaction results (with hash/status) and management API results (with ok flag), providing clearer output in non-debug mode.
167-172: These contract ID defaults are appropriate for this smoke test script. Both theno_auth_bump()andwrite_with_address_auth()methods are confirmed to exist in the smoke contract implementation atcontracts/smoke-contract/src/lib.rs, and the contract is maintained within this repository. The code already supports overriding these defaults via CLI arguments or environment variables.
Comment on lines
+259
to
+311
| { | ||
| id: 'mgmt-api', | ||
| label: 'Management API: fee limits CRUD', | ||
| run: async ({ client, apiKey, adminSecret, debug }) => { | ||
| if (!adminSecret) { | ||
| console.log(' ⏭ Skipped (requires --admin-secret)'); | ||
| return; | ||
| } | ||
|
|
||
| const formatStroops = (s: number) => `${s.toLocaleString()} stroops (${(s / 10_000_000).toFixed(7)} XLM)`; | ||
|
|
||
| // 1. Get current fee usage | ||
| const usage = await client.getFeeUsage(apiKey); | ||
| printResult('getFeeUsage', { success: true, data: usage }, debug); | ||
| if (!debug) console.log(` consumed: ${formatStroops(usage.consumed)}`); | ||
|
|
||
| // 2. Get current fee limit | ||
| const limitBefore = await client.getFeeLimit(apiKey); | ||
| printResult('getFeeLimit', { success: true, data: limitBefore }, debug); | ||
| if (!debug) console.log(` limit: ${limitBefore.limit ? formatStroops(limitBefore.limit) : 'unlimited'}`); | ||
|
|
||
| // 3. Set a custom fee limit (5 XLM) | ||
| const testLimit = 50_000_000; | ||
| const setResult = await client.setFeeLimit(apiKey, testLimit); | ||
| if (!setResult.ok || setResult.limit !== testLimit) { | ||
| throw new Error(`setFeeLimit failed: expected ${testLimit}, got ${setResult.limit}`); | ||
| } | ||
| printResult('setFeeLimit', { success: true, data: setResult }, debug); | ||
| if (!debug) console.log(` limit: ${formatStroops(setResult.limit)}`); | ||
|
|
||
| // 4. Verify limit was set | ||
| const limitAfterSet = await client.getFeeLimit(apiKey); | ||
| if (limitAfterSet.limit !== testLimit) { | ||
| throw new Error(`getFeeLimit after set: expected ${testLimit}, got ${limitAfterSet.limit}`); | ||
| } | ||
| printResult('getFeeLimit (verify set)', { success: true, data: limitAfterSet }, debug); | ||
|
|
||
| // 5. Delete the custom limit | ||
| const deleteResult = await client.deleteFeeLimit(apiKey); | ||
| if (!deleteResult.ok) { | ||
| throw new Error('deleteFeeLimit failed'); | ||
| } | ||
| printResult('deleteFeeLimit', { success: true, data: deleteResult }, debug); | ||
|
|
||
| // 6. Verify limit was deleted | ||
| const limitAfterDelete = await client.getFeeLimit(apiKey); | ||
| if (limitAfterDelete.limit === testLimit) { | ||
| throw new Error('getFeeLimit after delete: custom limit still present'); | ||
| } | ||
| printResult('getFeeLimit (verify deleted)', { success: true, data: limitAfterDelete }, debug); | ||
| if (!debug) console.log(` limit: ${limitAfterDelete.limit ? formatStroops(limitAfterDelete.limit) : 'unlimited'}`); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Save and restore initial state to avoid side effects.
The management API test modifies state (sets and deletes fee limits) without restoring the original state. If a custom limit exists before the test runs, it will be lost after the test completes. This can affect subsequent tests or ongoing operations.
Additionally, the verification at lines 305-307 only checks that the test limit was removed, not that the original state was restored.
🔎 Apply this diff to save and restore the initial state:
run: async ({ client, apiKey, adminSecret, debug }) => {
if (!adminSecret) {
console.log(' ⏭ Skipped (requires --admin-secret)');
return;
}
const formatStroops = (s: number) => `${s.toLocaleString()} stroops (${(s / 10_000_000).toFixed(7)} XLM)`;
// 1. Get current fee usage
const usage = await client.getFeeUsage(apiKey);
printResult('getFeeUsage', { success: true, data: usage }, debug);
if (!debug) console.log(` consumed: ${formatStroops(usage.consumed)}`);
// 2. Get current fee limit (save for restoration)
const limitBefore = await client.getFeeLimit(apiKey);
printResult('getFeeLimit', { success: true, data: limitBefore }, debug);
if (!debug) console.log(` limit: ${limitBefore.limit ? formatStroops(limitBefore.limit) : 'unlimited'}`);
+ const originalLimit = limitBefore.limit;
// 3. Set a custom fee limit (5 XLM)
const testLimit = 50_000_000;
const setResult = await client.setFeeLimit(apiKey, testLimit);
if (!setResult.ok || setResult.limit !== testLimit) {
throw new Error(`setFeeLimit failed: expected ${testLimit}, got ${setResult.limit}`);
}
printResult('setFeeLimit', { success: true, data: setResult }, debug);
if (!debug) console.log(` limit: ${formatStroops(setResult.limit)}`);
// 4. Verify limit was set
const limitAfterSet = await client.getFeeLimit(apiKey);
if (limitAfterSet.limit !== testLimit) {
throw new Error(`getFeeLimit after set: expected ${testLimit}, got ${limitAfterSet.limit}`);
}
printResult('getFeeLimit (verify set)', { success: true, data: limitAfterSet }, debug);
// 5. Delete the custom limit
const deleteResult = await client.deleteFeeLimit(apiKey);
if (!deleteResult.ok) {
throw new Error('deleteFeeLimit failed');
}
printResult('deleteFeeLimit', { success: true, data: deleteResult }, debug);
// 6. Verify limit was deleted
const limitAfterDelete = await client.getFeeLimit(apiKey);
if (limitAfterDelete.limit === testLimit) {
throw new Error('getFeeLimit after delete: custom limit still present');
}
printResult('getFeeLimit (verify deleted)', { success: true, data: limitAfterDelete }, debug);
if (!debug) console.log(` limit: ${limitAfterDelete.limit ? formatStroops(limitAfterDelete.limit) : 'unlimited'}`);
+
+ // 7. Restore original state
+ if (originalLimit !== undefined && originalLimit !== null) {
+ const restoreResult = await client.setFeeLimit(apiKey, originalLimit);
+ if (!restoreResult.ok || restoreResult.limit !== originalLimit) {
+ console.warn(` ⚠ Failed to restore original limit: expected ${originalLimit}, got ${restoreResult.limit}`);
+ } else {
+ if (!debug) console.log(` restored original limit: ${formatStroops(originalLimit)}`);
+ }
+ }
},📝 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.
Suggested change
| { | |
| id: 'mgmt-api', | |
| label: 'Management API: fee limits CRUD', | |
| run: async ({ client, apiKey, adminSecret, debug }) => { | |
| if (!adminSecret) { | |
| console.log(' ⏭ Skipped (requires --admin-secret)'); | |
| return; | |
| } | |
| const formatStroops = (s: number) => `${s.toLocaleString()} stroops (${(s / 10_000_000).toFixed(7)} XLM)`; | |
| // 1. Get current fee usage | |
| const usage = await client.getFeeUsage(apiKey); | |
| printResult('getFeeUsage', { success: true, data: usage }, debug); | |
| if (!debug) console.log(` consumed: ${formatStroops(usage.consumed)}`); | |
| // 2. Get current fee limit | |
| const limitBefore = await client.getFeeLimit(apiKey); | |
| printResult('getFeeLimit', { success: true, data: limitBefore }, debug); | |
| if (!debug) console.log(` limit: ${limitBefore.limit ? formatStroops(limitBefore.limit) : 'unlimited'}`); | |
| // 3. Set a custom fee limit (5 XLM) | |
| const testLimit = 50_000_000; | |
| const setResult = await client.setFeeLimit(apiKey, testLimit); | |
| if (!setResult.ok || setResult.limit !== testLimit) { | |
| throw new Error(`setFeeLimit failed: expected ${testLimit}, got ${setResult.limit}`); | |
| } | |
| printResult('setFeeLimit', { success: true, data: setResult }, debug); | |
| if (!debug) console.log(` limit: ${formatStroops(setResult.limit)}`); | |
| // 4. Verify limit was set | |
| const limitAfterSet = await client.getFeeLimit(apiKey); | |
| if (limitAfterSet.limit !== testLimit) { | |
| throw new Error(`getFeeLimit after set: expected ${testLimit}, got ${limitAfterSet.limit}`); | |
| } | |
| printResult('getFeeLimit (verify set)', { success: true, data: limitAfterSet }, debug); | |
| // 5. Delete the custom limit | |
| const deleteResult = await client.deleteFeeLimit(apiKey); | |
| if (!deleteResult.ok) { | |
| throw new Error('deleteFeeLimit failed'); | |
| } | |
| printResult('deleteFeeLimit', { success: true, data: deleteResult }, debug); | |
| // 6. Verify limit was deleted | |
| const limitAfterDelete = await client.getFeeLimit(apiKey); | |
| if (limitAfterDelete.limit === testLimit) { | |
| throw new Error('getFeeLimit after delete: custom limit still present'); | |
| } | |
| printResult('getFeeLimit (verify deleted)', { success: true, data: limitAfterDelete }, debug); | |
| if (!debug) console.log(` limit: ${limitAfterDelete.limit ? formatStroops(limitAfterDelete.limit) : 'unlimited'}`); | |
| }, | |
| }, | |
| { | |
| id: 'mgmt-api', | |
| label: 'Management API: fee limits CRUD', | |
| run: async ({ client, apiKey, adminSecret, debug }) => { | |
| if (!adminSecret) { | |
| console.log(' ⏭ Skipped (requires --admin-secret)'); | |
| return; | |
| } | |
| const formatStroops = (s: number) => `${s.toLocaleString()} stroops (${(s / 10_000_000).toFixed(7)} XLM)`; | |
| // 1. Get current fee usage | |
| const usage = await client.getFeeUsage(apiKey); | |
| printResult('getFeeUsage', { success: true, data: usage }, debug); | |
| if (!debug) console.log(` consumed: ${formatStroops(usage.consumed)}`); | |
| // 2. Get current fee limit (save for restoration) | |
| const limitBefore = await client.getFeeLimit(apiKey); | |
| printResult('getFeeLimit', { success: true, data: limitBefore }, debug); | |
| if (!debug) console.log(` limit: ${limitBefore.limit ? formatStroops(limitBefore.limit) : 'unlimited'}`); | |
| const originalLimit = limitBefore.limit; | |
| // 3. Set a custom fee limit (5 XLM) | |
| const testLimit = 50_000_000; | |
| const setResult = await client.setFeeLimit(apiKey, testLimit); | |
| if (!setResult.ok || setResult.limit !== testLimit) { | |
| throw new Error(`setFeeLimit failed: expected ${testLimit}, got ${setResult.limit}`); | |
| } | |
| printResult('setFeeLimit', { success: true, data: setResult }, debug); | |
| if (!debug) console.log(` limit: ${formatStroops(setResult.limit)}`); | |
| // 4. Verify limit was set | |
| const limitAfterSet = await client.getFeeLimit(apiKey); | |
| if (limitAfterSet.limit !== testLimit) { | |
| throw new Error(`getFeeLimit after set: expected ${testLimit}, got ${limitAfterSet.limit}`); | |
| } | |
| printResult('getFeeLimit (verify set)', { success: true, data: limitAfterSet }, debug); | |
| // 5. Delete the custom limit | |
| const deleteResult = await client.deleteFeeLimit(apiKey); | |
| if (!deleteResult.ok) { | |
| throw new Error('deleteFeeLimit failed'); | |
| } | |
| printResult('deleteFeeLimit', { success: true, data: deleteResult }, debug); | |
| // 6. Verify limit was deleted | |
| const limitAfterDelete = await client.getFeeLimit(apiKey); | |
| if (limitAfterDelete.limit === testLimit) { | |
| throw new Error('getFeeLimit after delete: custom limit still present'); | |
| } | |
| printResult('getFeeLimit (verify deleted)', { success: true, data: limitAfterDelete }, debug); | |
| if (!debug) console.log(` limit: ${limitAfterDelete.limit ? formatStroops(limitAfterDelete.limit) : 'unlimited'}`); | |
| // 7. Restore original state | |
| if (originalLimit !== undefined && originalLimit !== null) { | |
| const restoreResult = await client.setFeeLimit(apiKey, originalLimit); | |
| if (!restoreResult.ok || restoreResult.limit !== originalLimit) { | |
| console.warn(` ⚠ Failed to restore original limit: expected ${originalLimit}, got ${restoreResult.limit}`); | |
| } else { | |
| if (!debug) console.log(` restored original limit: ${formatStroops(originalLimit)}`); | |
| } | |
| } | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In scripts/smoke.ts around lines 259 to 311, the test mutates the fee limit
without restoring the original value; capture the initial limit (and whether it
was "unlimited") before changing anything, run the set/delete/verify steps
inside a try block, and in a finally block restore the original state: if there
was an original numeric limit re-apply it, otherwise ensure the custom limit is
deleted; update the final verification to assert that the post-test state
matches the captured original value rather than only checking that the test
value was removed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
--admin-secretand--log-fees-spentCLI flags for management operations.Improvements
✏️ Tip: You can customize this high-level summary in your review settings.