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.

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

avm-abi: Update functions migrated to avm-abi library #4979

Merged
merged 9 commits into from
Jan 19, 2023

Conversation

AlgoStephenAkiki
Copy link
Contributor

@AlgoStephenAkiki AlgoStephenAkiki commented Jan 6, 2023

Summary

Several functions have been migrated to the avm-abi library. This pull request updates the code to use the functions in their new location.

Test Plan

Existing regression tests.

@AlgoStephenAkiki AlgoStephenAkiki changed the title Changed to use avm New Feature: Changed to use avm Jan 6, 2023
@AlgoStephenAkiki AlgoStephenAkiki self-assigned this Jan 6, 2023
@AlgoStephenAkiki AlgoStephenAkiki marked this pull request as ready for review January 6, 2023 21:33
@AlgoStephenAkiki AlgoStephenAkiki force-pushed the 1328-remove-logic branch 2 times, most recently from fd7e913 to bf6d8a7 Compare January 7, 2023 01:18
@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Merging #4979 (acaa2d1) into master (4631571) will decrease coverage by 0.04%.
The diff coverage is 10.00%.

@@            Coverage Diff             @@
##           master    #4979      +/-   ##
==========================================
- Coverage   53.68%   53.65%   -0.04%     
==========================================
  Files         432      431       -1     
  Lines       54068    53998      -70     
==========================================
- Hits        29029    28975      -54     
+ Misses      22793    22784       -9     
+ Partials     2246     2239       -7     
Impacted Files Coverage Δ
cmd/goal/interact.go 3.62% <0.00%> (ø)
daemon/algod/api/server/v2/handlers.go 0.82% <0.00%> (ø)
data/transactions/logic/box.go 94.66% <ø> (-0.55%) ⬇️
ledger/internal/applications.go 3.87% <0.00%> (ø)
libgoal/libgoal.go 2.59% <ø> (ø)
cmd/goal/application.go 17.78% <50.00%> (ø)
ledger/roundlru.go 90.56% <0.00%> (-5.67%) ⬇️
catchup/service.go 69.80% <0.00%> (-0.73%) ⬇️
network/wsPeer.go 68.32% <0.00%> (-0.48%) ⬇️
ledger/acctupdates.go 69.24% <0.00%> (+0.24%) ⬆️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

go.mod Outdated Show resolved Hide resolved
@algorandskiy algorandskiy marked this pull request as draft January 11, 2023 18:18
@algorandskiy
Copy link
Contributor

Converted to Draft to prevent accidental merge before AVM-ABI release

ledger/acctdeltas_test.go Outdated Show resolved Hide resolved
ledger/acctupdates_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Please fix the import order, otherwise LGTM

@AlgoStephenAkiki AlgoStephenAkiki marked this pull request as ready for review January 17, 2023 16:46
@winder winder changed the title New Feature: Changed to use avm avm-abi: Update functions migrated to avm-abi library Jan 18, 2023
go.mod Outdated
@@ -39,6 +38,7 @@ require (
)

require (
github.com/algorand/avm-abi v0.2.0 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this indirect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. I added this via go get but updated the mod file manually to be a direct require.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can run go mod tidy to have go update the dependencies to match their usage in the codebase. I'd recommend still running it locally, since it might reorder the dependencies

winder
winder previously approved these changes Jan 18, 2023
@winder winder requested review from bbroder-algo and removed request for michaeldiamant January 18, 2023 13:28
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Replacement looks correct to me, but I do have a (likely inconsequential) comment about the go.sum changes

go.sum Outdated Show resolved Hide resolved
@winder winder merged commit a9fe2c7 into algorand:master Jan 19, 2023
@AlgoStephenAkiki AlgoStephenAkiki deleted the 1328-remove-logic branch January 19, 2023 16:12
algorandskiy added a commit to algorandskiy/go-algorand that referenced this pull request Jan 19, 2023
algorandskiy added a commit that referenced this pull request Jan 19, 2023
Copy link
Contributor

@bbroder-algo bbroder-algo left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants