-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat: basic blob-registry contract #116
Conversation
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.
Looks great, just needs some small changes
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.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @palozano)
1bb78b6
to
b7fcb3d
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.
Reviewed 1 of 7 files at r2, 9 of 9 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @palozano)
contracts/blob-registry/src/lib.rs
line 102 at r1 (raw file):
Previously, palozano (Lozano) wrote…
And should we do something with the vector of transaction IDs (I assume they are strings)? Store them or do nothing for now?
For now, we do nothing. Eventually we will have onchain priority queue
contracts/blob-registry/src/lib.rs
line 110 at r1 (raw file):
Previously, palozano (Lozano) wrote…
update_owner()
is only checkingnew != old
, so I think we still needrequire_owner()
to check that the only one calling this method is the owner, right?
I see, is it still asserting one yocto too? Or has that changed. IIRC jacob asserted one yocto on most sdkctl calls
contracts/blob-registry/src/lib.rs
line 39 at r3 (raw file):
type Priority = u32; type Maintainer = Vec<u8>; type TransactionId = String;
This is a problem if it's a string. At best, I'd be happy to take 32-byte arrays if the crypto hash isn't available. It's even better to use a custom serializer/deserializer such that they are hex-encoded by Serde. Check NEARX or libs (don't use a lib since they're usually bloated and near contracts can now only be 1.5mb.)
contracts/blob-registry/src/lib.rs
line 117 at r3 (raw file):
env::panic_str(ERR_NAMESPACE_EXISTS); } else { // when the deposit is enough
gud
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.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dndll)
contracts/blob-registry/src/lib.rs
line 102 at r1 (raw file):
Previously, dndll (Don) wrote…
For now, we do nothing. Eventually we will have onchain priority queue
Done.
contracts/blob-registry/src/lib.rs
line 110 at r1 (raw file):
Previously, dndll (Don) wrote…
I see, is it still asserting one yocto too? Or has that changed. IIRC jacob asserted one yocto on most sdkctl calls
As far as I've seen, there's no assertion for one yocto. This is the implementation of the method.
Code snippet:
fn update_owner(&mut self, new: Option<AccountId>) {
let owner = Self::slot_owner();
let old = owner.read();
if old != new {
OwnerEvent::Transfer {
old,
new: new.clone(),
}
.emit();
self.update_owner_unchecked(new);
}
}
contracts/blob-registry/src/lib.rs
line 39 at r3 (raw file):
Previously, dndll (Don) wrote…
This is a problem if it's a string. At best, I'd be happy to take 32-byte arrays if the crypto hash isn't available. It's even better to use a custom serializer/deserializer such that they are hex-encoded by Serde. Check NEARX or libs (don't use a lib since they're usually bloated and near contracts can now only be 1.5mb.)
I will go with the custom de/serializer
contracts/blob-registry/src/lib.rs
line 117 at r3 (raw file):
Previously, dndll (Don) wrote…
gud
Done.
050bd78
to
ddfa748
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.
Reviewed 9 of 9 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @palozano)
92490d0
to
e01a31e
Compare
eb34ef8
to
26379e4
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.
Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @palozano)
New
blob-registry
contract.This change is