- 
                Notifications
    
You must be signed in to change notification settings  - Fork 2.2k
 
          Update the way that keys are build in views obtained by linera-views-derive.
          #4419
        
          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
  
    Update the way that keys are build in views obtained by linera-views-derive.
  
  #4419
              Conversation
linera-views-derive.
      f8c7730    to
    c5433dd      
    Compare
  
            
          
                linera-views-derive/src/snapshots/linera_views_derive__tests__test_generate_view_code_C.snap
              
                Outdated
          
            Show resolved
            Hide resolved
        
      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.
As said above, this should be two PRs so that we can update the SDK 0.15.
c96a821    to
    ddc1527      
    Compare
  
    
          
 Created another PR #4452 which does the addition of the prefix   | 
    
        
          
                linera-views-derive/src/lib.rs
              
                Outdated
          
        
      | } else { | ||
| quote! { | ||
| let index = #idx_lit; | ||
| let base_key = context.base_key().derive_tag_key(linera_views::views::MIN_VIEW_TAG, &index)?; | 
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.
Can we make sure that the integer type is (locally) visible when we use derive_tag_key on integers? Those types are part of the protocol and any change would be breaking.
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.
(ok it looks like this is maybe the last place where it's ambiguous)
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.
You are very right. I added a bound of 65536 on the number of entries.
So, the key is either u8 or u16.
| let mut pos = 0; | ||
| let index = 0; | ||
| let pos_next = pos + RegisterView::<C, usize>::NUM_INIT_KEYS; | ||
| let index_byte = 0u8; | 
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.
nit: Here and in other places. We don't really need _byte. (We need 0u8)
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.
I removed the "_byte" and added "_u8" / "_u16" when adequate.
Now the generated code does not contain the "_byte", but it does contain "0u8" / "0u16".
ddc1527    to
    96e06cb      
    Compare
  
    96e06cb    to
    71cb6e0      
    Compare
  
    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.
Thanks!
Motivation
It has recently been indicated in PR #4347 that the use
ChainIdin a message would be a waste of space.But there are other wastage of key space that can be reduced.
Fixes #2391
Proposal
We check for the number of entries in views obtained by
linera-views-derive. If lower than 256, we convert tou8.It is likely that we need to remove as well the
linera_views::views::MIN_VIEW_TAG.Test Plan
CI.
Release Plan
It is definitely breaking the testnet.
Links
None