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.

Sign up for GitHub

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

Jump to bottom

Feat/stacks signer fee estimate #4583

Merged
merged 13 commits into from Apr 5, 2024
Merged

Feat/stacks signer fee estimate #4583

merged 13 commits into from Apr 5, 2024
+397 −151

Conversation

jferrant
Copy link
Collaborator

Closes #4543

@jferrant jferrant mentioned this pull request Mar 22, 2024
Closed
@jferrant jferrant marked this pull request as ready for review March 22, 2024 22:00
@codecov Codecov
Copy link

codecov bot commented Mar 22, 2024 •
edited

Codecov Report

Attention: Patch coverage is 95.63953% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 77.86%. Comparing base (d54b67c) to head (76b610b).

Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #4583      +/-   ##
==========================================
- Coverage   83.31%   77.86%   -5.45%     
==========================================
  Files         470      470              
  Lines      332768   332977     +209     
  Branches      317      317              
==========================================
- Hits       277247   259286   -17961     
- Misses      55513    73683   +18170     
  Partials        8        8
Files Coverage Δ
stacks-signer/src/client/mod.rs 91.91% <100.00%> (-7.02%) ⬇️
stacks-signer/src/client/stackerdb.rs 89.17% <100.00%> (-0.10%) ⬇️
stacks-signer/src/runloop.rs 93.56% <100.00%> (-0.09%) ⬇️
stacks-signer/src/signerdb.rs 75.29% <100.00%> (-15.69%) ⬇️
testnet/stacks-node/src/tests/signer.rs 95.64% <100.00%> (-0.08%) ⬇️
stacks-signer/src/cli.rs 5.96% <0.00%> (-57.06%) ⬇️
stacks-signer/src/main.rs 28.08% <0.00%> (-7.93%) ⬇️
stacks-signer/src/client/stacks_client.rs 93.88% <97.11%> (+5.66%) ⬆️
stacks-signer/src/signer.rs 73.81% <94.00%> (+0.25%) ⬆️
stacks-signer/src/config.rs 83.22% <95.72%> (-5.25%) ⬇️

... and 182 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d54b67c...76b610b. Read the comment docs.

@jferrant jferrant force-pushed the feat/stacks-signer-fee-estimate branch from 851c22e to da62ad6 Compare March 26, 2024 15:47
jcnelson
jcnelson previously approved these changes Mar 26, 2024
View reviewed changes
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just had a couple of questions that need to be answered prior to merging

obycode
obycode previously approved these changes Mar 26, 2024
View reviewed changes
@jferrant jferrant dismissed stale reviews from obycode and jcnelson via b9bf740 March 27, 2024 14:24
jcnelson
jcnelson previously approved these changes Mar 27, 2024
View reviewed changes
@jferrant jferrant mentioned this pull request Mar 27, 2024
Merged
hstove
hstove requested changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

Everything looks good except the logging of the whole config at startup - I really think we shouldn't have that logged as-is.

stacks-signer/src/client/stacks_client.rs Show resolved Hide resolved
stacks-signer/src/config.rs Outdated Show resolved Hide resolved
stacks-signer/src/main.rs Outdated Show resolved Hide resolved
hstove
hstove previously approved these changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

LGTM!

kantai
kantai previously approved these changes Mar 29, 2024
View reviewed changes
jcnelson
jcnelson previously approved these changes Mar 29, 2024
View reviewed changes
@jferrant
Remove unnecessary retries throughout code
ee5c726 
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant
Add get_medium_estimated_fee_ustx to get estimated fee from stacks node
7a970ac 
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant
Seperate unsigned and signed transaction building and update tests
abfc68a 
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant
Update stacks-signer to attempt to estimate the transaction fee if a …
00fd7db 
…user sets max_tx_fee_ustx in the config

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant
CRC: remove dead path and update debug message to a warn
a112bf5 
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant
CRC: add config tests and remove config log
110d04e 
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant dismissed stale reviews from jcnelson, kantai, and hstove via 110d04e April 2, 2024 13:00
@jferrant jferrant force-pushed the feat/stacks-signer-fee-estimate branch from b958325 to 110d04e Compare April 2, 2024 13:00
@jferrant
Merge branch 'next' of https://github.com/stacks-network/stacks-core …
d29dde3 
…into feat/stacks-signer-fee-estimate
@jferrant
Fix bad merge
3c4e124 
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@AshtonStephens AshtonStephens mentioned this pull request Apr 3, 2024
Open
jcnelson
jcnelson previously approved these changes Apr 3, 2024
View reviewed changes
kantai
kantai previously approved these changes Apr 3, 2024
View reviewed changes
@jferrant
Merge branch 'next' of https://github.com/stacks-network/stacks-core …
21368aa 
…into feat/stacks-signer-fee-estimate
@jferrant
CRC: change debug to info for pre epoch 3.0 cycles requiring a fee
1028135 
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant dismissed stale reviews from kantai and jcnelson via 1028135 April 3, 2024 18:27
@jferrant jferrant requested review from jcnelson and kantai April 3, 2024 18:27
@jferrant
Merge branch 'next' of https://github.com/stacks-network/stacks-core …
972b631 
…into feat/stacks-signer-fee-estimate
@jferrant
Fix bad merge
85eb351 
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant
Fix bad merge 2
76b610b 
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@obycode
Copy link
Contributor

obycode commented Apr 5, 2024

Should we go ahead and merge this, or re-target it to develop?

@jferrant jferrant added this pull request to the merge queue Apr 5, 2024
Merged via the queue into next with commit 8d4da57 Apr 5, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcnelson jcnelson jcnelson approved these changes

@kantai kantai kantai approved these changes

@obycode obycode Awaiting requested review from obycode

@hstove hstove Awaiting requested review from hstove

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jferrant @obycode @jcnelson @hstove @kantai