-
Notifications
You must be signed in to change notification settings - Fork 170
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
fix(sdkclient): acc sequence setting #2375
Conversation
WalkthroughThe codebase has undergone a refactoring where the method Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChat with CodeRabbit Bot (
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2375 +/- ##
==========================================
- Coverage 75.38% 70.03% -5.36%
==========================================
Files 100 174 +74
Lines 8025 13027 +5002
==========================================
+ Hits 6050 9123 +3073
- Misses 1589 3299 +1710
- Partials 386 605 +219
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- client/client.go (1 hunks)
- sdkclient/tx/client.go (1 hunks)
- sdkclient/tx/wasm.go (1 hunks)
- tests/e2e/setup/utils.go (1 hunks)
Files skipped from review due to trivial changes (1)
- client/client.go
Additional comments: 3
sdkclient/tx/wasm.go (1)
- 50-53: The separation of setting the account sequence from the transaction broadcasting logic using
SetAccSeq
is a logical change. Ensure thatSetAccSeq
is correctly implemented and that the account sequence is being used correctly in theBroadcastTx
method.sdkclient/tx/client.go (1)
- 141-142: The modification to
SetAccSeq
to directly set the account sequence in thetxFactory
is consistent with the PR objectives. Verify that thetxFactory
is correctly updated and that no other parts of the code rely on the old behavior of returning a newClient
instance.Verification successful
To verify the usage of
WithAccSeq
intests/util/cw_util.go
andsdkclient/tx/wasm.go
, we will look at the method definitions and usages in these files to understand if they have been updated or if they are separate concerns.
The analysis of
tests/util/cw_util.go
andsdkclient/tx/wasm.go
files indicates that:
- The
WasmExecContractWithAccSeq
method insdkclient/tx/wasm.go
has been updated to use the newSetAccSeq
method, aligning with the PR's objectives.- The test utility methods
CWExecuteWithSeqAndAsync
andCWExecuteWithSeqAndAsyncResp
intests/util/cw_util.go
are using the updatedWasmExecContractWithAccSeq
method, ensuring consistency with the new changes.Therefore, the remaining references to
WithAccSeq
are not an issue as they have been properly updated to work with the new method.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that no other parts of the code rely on the old WithAccSeq method. rg --type go 'WithAccSeq'Length of output: 436
Script:
#!/bin/bash # Check the method definitions and usages of WithAccSeq in tests/util/cw_util.go cat tests/util/cw_util.go # Check the method definitions and usages of WithAccSeq in sdkclient/tx/wasm.go cat sdkclient/tx/wasm.goLength of output: 5172
tests/e2e/setup/utils.go (1)
- 314-314: The change from
WithAccSeq
toSetAccSeq
in theBroadcastTxWithRetry
function aligns with the PR objectives. Verify that the function correctly handles the account sequence and that the retry logic is still valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre Approving , Can you check PR link to the https://github.com/umee-network/umee/pull/2375/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR66-R70
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 70-70: The API Breaking change is correctly documented.
Description
Summary by CodeRabbit
Refactor
Tests