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

Language Server updates #1295

Merged
merged 14 commits into from
Dec 10, 2021
Merged

Conversation

MaxStalker
Copy link
Contributor

@MaxStalker MaxStalker commented Dec 3, 2021

Closes #1294

Description

This PR will:

  • update dependencies to latest versions
  • fix signing mechanism without changes to ServiceAccount
  • allow multi sign transactions using pragma keyword

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@MaxStalker MaxStalker force-pushed the max/language-server-update-dependencies branch from 5ab5da2 to a759a95 Compare December 3, 2021 16:15
@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit a9ef2ce
The command for i in {1..N}; do go test ./... -run=XXX -bench=. -shuffle=on; done was used.
Bench tests were run a total of 7 times on each branch.

Results

old.txtnew.txt
time/opdelta
RuntimeFungibleTokenTransfer-21.49ms ±19%1.56ms ±18%~(p=0.535 n=7+7)
RuntimeResourceDictionaryValues-216.6ms ± 3%16.5ms ± 5%~(p=0.805 n=7+7)
ParseArray-224.3ms ± 5%23.7ms ± 5%~(p=0.165 n=7+7)
ParseDeploy/byte_array-236.3ms ± 2%36.3ms ± 6%~(p=0.731 n=6+7)
ParseDeploy/decode_hex-21.51ms ± 7%1.50ms ± 3%~(p=0.620 n=7+7)
ParseInfix-225.4µs ± 2%25.0µs ± 1%~(p=0.051 n=7+6)
ParseFungibleToken-2471µs ± 3%468µs ± 5%~(p=0.534 n=6+7)
QualifiedIdentifierCreation/One_level-23.35ns ± 8%3.31ns ± 4%~(p=0.535 n=7+7)
ContractInterfaceFungibleToken-249.4µs ± 6%48.8µs ± 3%~(p=0.710 n=7+7)
CheckContractInterfaceFungibleTokenConformance-2175µs ± 3%170µs ± 5%~(p=0.053 n=7+7)
InterpretRecursionFib-22.82ms ± 5%2.79ms ± 6%~(p=0.620 n=7+7)
NewInterpreter/new_interpreter-21.23µs ± 7%1.19µs ± 2%~(p=0.073 n=7+7)
NewInterpreter/new_sub-interpreter-22.24µs ± 5%2.21µs ± 3%~(p=1.000 n=7+7)
QualifiedIdentifierCreation/Three_levels-2179ns ± 3%174ns ± 3%−2.78%(p=0.020 n=6+7)
 
alloc/opdelta
RuntimeFungibleTokenTransfer-2238kB ± 0%238kB ± 0%~(p=0.974 n=7+7)
RuntimeResourceDictionaryValues-24.04MB ± 0%4.04MB ± 0%~(p=0.929 n=7+7)
QualifiedIdentifierCreation/One_level-20.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-264.0B ± 0%64.0B ± 0%~(all equal)
ContractInterfaceFungibleToken-226.5kB ± 0%26.5kB ± 0%~(all equal)
CheckContractInterfaceFungibleTokenConformance-265.7kB ± 0%65.7kB ± 0%~(p=1.000 n=7+7)
InterpretRecursionFib-21.24MB ± 0%1.24MB ± 0%~(all equal)
NewInterpreter/new_interpreter-2720B ± 0%720B ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-21.11kB ± 0%1.11kB ± 0%~(all equal)
 
allocs/opdelta
RuntimeFungibleTokenTransfer-24.53k ± 0%4.53k ± 0%~(p=0.612 n=7+7)
RuntimeResourceDictionaryValues-2102k ± 0%102k ± 0%~(p=0.359 n=7+7)
QualifiedIdentifierCreation/One_level-20.00 0.00 ~(all equal)
QualifiedIdentifierCreation/Three_levels-22.00 ± 0%2.00 ± 0%~(all equal)
ContractInterfaceFungibleToken-2457 ± 0%457 ± 0%~(all equal)
CheckContractInterfaceFungibleTokenConformance-21.07k ± 0%1.07k ± 0%~(all equal)
InterpretRecursionFib-225.0k ± 0%25.0k ± 0%~(all equal)
NewInterpreter/new_interpreter-211.0 ± 0%11.0 ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-232.0 ± 0%32.0 ± 0%~(all equal)
 

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Changes with proposed changes look good!

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2021

Codecov Report

Merging #1295 (e981d9d) into master (a9ef2ce) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head e981d9d differs from pull request most recent head 62fe373. Consider uploading reports for the commit 62fe373 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1295      +/-   ##
==========================================
- Coverage   77.41%   77.39%   -0.02%     
==========================================
  Files         279      279              
  Lines       36113    36048      -65     
==========================================
- Hits        27956    27899      -57     
+ Misses       7066     7062       -4     
+ Partials     1091     1087       -4     
Flag Coverage Δ
unittests 77.39% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
runtime/interpreter/value.go 80.11% <0.00%> (-0.11%) ⬇️

Continue to review full report at Codecov.

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

@MaxStalker
Copy link
Contributor Author

@SupunS can you check why test is failing?..

@turbolent
Copy link
Member

@MaxStalker CI is failing because it checks that all generated files are checked in, which isn't the case on master. This isn't your fault, I'm fixing on master and will rerun CI for you

Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

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

Go changes look fine to me. Also, already been reviewed by others with more contextual knowledge. 🎉

@MaxStalker MaxStalker merged commit 24031a9 into master Dec 10, 2021
@MaxStalker MaxStalker deleted the max/language-server-update-dependencies branch December 10, 2021 20:14
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.

Language Server - Switching Active Account and Multi Sign Transactions
6 participants