evg_may5_review_qs.org

Questions regarding evgeny’s contract review

Factory

Why env::pred(), and not not env::current_account_id()

https://github.com/Mintbase/mb-contracts/pull/29/commits/5282cc042ccf40e7d8012ec90cdc5918d9840c25#diff-904ee840186925bca41dc3f8d92b89e0752144be75c7bb4b9859496a27a7db10R151

Why does this trait exist twice?

https://github.com/Mintbase/mb-contracts/pull/29/commits/5282cc042ccf40e7d8012ec90cdc5918d9840c25#diff-904ee840186925bca41dc3f8d92b89e0752144be75c7bb4b9859496a27a7db10R166

Market

Check my assumption:

a Predecessor will always have at least one ‘.’, so the following is safe:

/// If pred is "sub.base.near" or "base.near", return "base.near"
pub(crate) fn get_pred_base_account(&self) -> near_sdk::AccountId {
  let maybe_account = near_sdk::env::predecessor_account_id()
    // splits "sub.base.near" into ["abc", "base.near"]
    // and "base.near" into ["base", "near"]
    .splitn(2, '.')
    .nth(1)
    // so this unwrap is safe
    .unwrap()
    .to_string();
  if maybe_account.contains('.') {
    // stripped off subaccount, return base account
    maybe_account
  } else {
    // pred was already a base account
    env::predecessor_account_id()
  }
}

“But then change the logic of withdrawable to be deposit_required”: Clever.

Store

Line 439: Do you need to delist the token to return the current offer?

This comment seems out of place. The store doesn’t know anything about listings” or current_offers. When a token is burned, the market doesn’t get a notice (it used to, in a prior revision).

Core: I think you mean that there should be an assertion…

validating that there isn’t already a split_owner in the daycare.

Something like this:

// save to `split_owner_daycare` in case token is rolled back to original state
// check if there already is a split owner in the daycare to avoid race-cond
assert!(self.split_owners_daycare.get(&token_idu64).is_none());
if let Some(split_owners) = token.split_owners.take() {
  self
    .split_owners_daycare
    .insert(&token_idu64, &split_owners);
}

Composeable promise logic:

On refactor of ext_nft_holder, a callback implementation might look like this:

  /// Helper to get the holder of a top-level token on another contract.
pub(crate) fn ext_nft_holder(&self, token_id: U64, account_id: AccountId) -> Option<String> {
  ext_on_compose::nft_holder(token_id, &account_id, NO_DEPOSIT, DEFAULT_GAS / 5).then(
    ext_self::ext_nft_holder_callback(&env::current_account_id(), NO_DEPOSIT, DEFAULT_GAS / 5),
  )
}

#[private]
pub fn ext_nft_holder_callback(&self) -> Option<String> {
  {
    match env::promise_result(0) {
      PromiseResult::Successful(acct) => Some(near_sdk::serde_json::from_slice(&acct).unwrap()),
      PromiseResult::NotReady => None,
      PromiseResult::Failed => None,
    }
  }
}

Except that the returned type from ext_self::ext_nft_holder_callback is Promise, not Option<String>, so this errors out. What’s the fix?

Loan: Line 60: This is not going to work, since there is no blocking promises. So the best way is to return some info for the front-end to check other contract.

Regarding this and the prior bullet point, I’d love to have a conversation to make sure I understand.