SmartTokenLabs / TokenScript

TokenScript schema, specs and paper
http://tokenscript.org
MIT License
242 stars 71 forks source link

Add origins in activity #373

Closed SmartLayer closed 4 years ago

SmartLayer commented 4 years ago

As per TokenScript weekly meeting #40, developers should base their Activity A3 project on the files provided in ERCs directory. Notice that this is based on PR #365 which will be merged in a few weeks since αW is released with a fix

hboon commented 4 years ago

Something to clear up before commenting on the JavaScript API changed to support this PR, instead of doing this:

web3.tokens.dataChanged = (oldTokens, updatedTokens, tokenIdCard) => {
    const currentTokenInstance = web3.tokens.data.currentInstance;
    document.getElementById(tokenIdCard).innerHTML = new Token(currentTokenInstance).render();
};

We should be doing this (i.e. not access web3.tokens.data.currentInstance):

web3.tokens.dataChanged = (oldTokens, updatedTokens, tokenIdCard) => {
    const currentTokenInstance = updatedTokens.currentInstance;
    document.getElementById(tokenIdCard).innerHTML = new Token(currentTokenInstance).render();
};

The latter assumes this:

{
  updatedTokens: {
    currentInstance: {
      tokenAttribute1: value,
      tokenAttribute2: value,
    }
  }
}

And for this PR

Let's just get rid of currentInstance once and for all, so web3 (under window) is not populated with currentInstance anymore:

{
  web3: {
    tokens: {
      data: {
        currentInstance: {
          tokenAttribute1: value,
          tokenAttribute2: value,
        }
      }
    }
  }
}

Instead, let's make the dataChanged handler work this way:

web3.tokens.dataChanged = (old, updated, tokenIdCard) => {
    const data = updated
    document.getElementById(tokenIdCard).innerHTML = new Token(data).render()
}

where updated (and old) looks like this:

updated = {
  token: {
    tokenAttribute1: value,
    tokenAttribute2: value,
  },
  card: {
    cardAttribute1: value,
    cardAttribute2: value,
  }
}

the developer can choose assign updated directly as props and thus access token attribute like this:

const tokenAttributeValue = this.props.token.tokenAttribute1
const cardAttributeValue = this.props.card.tokenAttribute1

And if they are sure the attribute names don't clash, this is easier to migrate to since only the dataChanged handler needs to be modified:

web3.tokens.dataChanged = (old, updated, tokenIdCard) => {
    const data = Object.assign({}, updated.token, updated.card)
    document.getElementById(tokenIdCard).innerHTML = new Token(data).render()
}

Then this is unchanged:

const attributeValue = this.props.tokenAttribute1 //or this.props.cardAttribute1

All of these assumes that we have at most 1 token, at most 1 card within each firing of dataChanged() Is that correct @colourful-land ?

hboon commented 4 years ago

So web3.tokens.dataChanged is the only place where we tuck things under web3. We can either change it now or next time. But it should be a very localised change (just that 1 line) when it happens.

hboon commented 4 years ago

@colourful-land have updated the JavaScript docs.

hboon commented 4 years ago

I suggest we preserve the value of updatedTokens.currentInstance for a while, i.e roughly this pseudo-code when injecting:

updatedTokens.currentInstance == updatedTokens.token + updatedTokens.card

so that the following JavaScript code in existing TokenScript files doesn't need to be updated immediately:

web3.tokens.dataChanged = (oldTokens, updatedTokens, tokenIdCard) => {
    const currentTokenInstance = updatedTokens.currentInstance;
    document.getElementById(tokenIdCard).innerHTML = new Token(currentTokenInstance).render();
};

cc @James-Sangalli

hboon commented 4 years ago
hboon commented 4 years ago

Merge branch 'master' into correct-violation-of-rfc4912+add-origins-i…

git rebase master instead? :)

SmartLayer commented 4 years ago

Merge branch 'master' into correct-violation-of-rfc4912+add-origins-i…

git rebase master instead? :)

Not sure if anyone has mod in local copy - also didn't like rebase

SmartLayer commented 4 years ago

@James-Sangalli :

if rebased this message wouldn't appear in git log @colourful-land Merge branch 'master' into correct-violation-of-rfc4912+add-origins-i…9d8061e

I don't know why it is a problem to be solved…

bitcoinwarrior1 commented 4 years ago

tokenAttribute1

@hboon I prefer this syntax for the web3 object as it separates them and prevents collisions (while also letting you know what is global and what is based on cards)

web3.tokens.dataChanged = (oldTokens, updated, tokenIdCard) => {
    document.getElementById(tokenIdCard).innerHTML = new Token(updated.token, updated.card).render();
};

constructor(token, card) {
        this.token = token;
        this.card = card;
}
hboon commented 4 years ago

tokenAttribute1

@hboon I prefer this syntax for the web3 object as it separates them and prevents collisions (while also letting you know what is global and what is based on cards)

web3.tokens.dataChanged = (oldTokens, updated, tokenIdCard) => {
    document.getElementById(tokenIdCard).innerHTML = new Token(updated.token, updated.card).render();
};

constructor(token, card) {
        this.token = token;
        this.card = card;
}

Up to you if that's a better way to write the JavaScript for the views. What you suggested is the same as the one I suggested for what the TokenScript engine exposes as an API, i.e there is updated.token and updated.card (and for the time being, hopefully briefly, the legacy updated.currentInstance) in here:

web3.tokens.dataChanged = (oldTokens, updated, tokenIdCard) => {
}
JamesSmartCell commented 4 years ago

This PR seems to cover a diverse range of updates.

  1. The dataChanged method - this is how it was always supposed to be working, right? So far it's been a slow evolution toward this.
  2. The origins in activity is a good refactor and will make the parsing code more concise - so, it's acceptable :)